Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfixes for source maps #2216

Merged
merged 2 commits into from
Oct 22, 2016
Merged

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Oct 21, 2016

Fixes #2204
Replaces #2208

  • Fixes parent selector mappings
  • Fixes media block/query mappings
  • Fixes range over binary expressions
  • Don't include semicolon for statics
  • Fixes variable assignment mappings

I have already deployed the changes to my source map inspector!

Parent selector mappings

SourceMap Inspector (click [inspect] and use keyboard arrows to navigate)

.foo {
  background: red;
  &-bar {
    background: green;
  }
}

Media Block expressions

SourceMap Inspector (click [inspect] and use keyboard arrows to navigate)

.foo {
  background: red;
  @media only screen {
    background: green;
  }
}

Binary expressions

SourceMap Inspector (click [inspect] and use keyboard arrows to navigate)

.foo {
  bar: (1px+3px);
}

Static values

SourceMap Inspector (click [inspect] and use keyboard arrows to navigate)

.foo {
  bar: green   ;
}

Variables (last assignment)

SourceMap Inspector (click [inspect] and use keyboard arrows to navigate)

$var: bar;
$var: (foo + $var);
test { val: $var; }

Copy link
Collaborator

@nschonni nschonni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, this is a decent resource beyond the spec https://github.com/mozilla/source-map

@@ -1527,6 +1546,9 @@ namespace Sass {
{
lex< static_value >();
Token str(lexed);
// static valuse always have trailing white-
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valuse -> values

// keep old parser state
s->pstate(pstate());
// append new tail
s->append(ctx, ss);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent

@mgreter mgreter force-pushed the bugfix/source-mappings branch from e7a3b7e to 8e80295 Compare October 21, 2016 21:59
@mgreter
Copy link
Contributor Author

mgreter commented Oct 21, 2016

thx @nschonni. Fixed the formatting and typo!

@mgreter mgreter force-pushed the bugfix/source-mappings branch from 8e80295 to 2137f8c Compare October 21, 2016 23:52
@mgreter mgreter added this to the 3.4 milestone Oct 22, 2016
@mgreter mgreter self-assigned this Oct 22, 2016
@mgreter mgreter force-pushed the bugfix/source-mappings branch 2 times, most recently from 7f51e3f to 51ff53f Compare October 22, 2016 01:31
@xzyfer
Copy link
Contributor

xzyfer commented Oct 22, 2016

Thank you @mgreter. Please try to keep PRs focused. This PR is address 4 different issues of differing merit.

I would prefer not merge the makefile change. The existing order is obvious and self documenting.

@xzyfer
Copy link
Contributor

xzyfer commented Oct 22, 2016

@mgreter I have cherry picked 314360e and 51ff53f on to master. Please back out f8d9dec, then feel free to ship this.

I'll be feature freezing master after this merge so I release a 3.4 RC this weekend.

@mgreter
Copy link
Contributor Author

mgreter commented Oct 22, 2016

Hmm, f8d9dec saves me 1 minute compile time. But whatever ...

@xzyfer
Copy link
Contributor

xzyfer commented Oct 22, 2016

I understand as a power user this is of benefit to us. My reasoning is

  • it's something that we can't guarantee the correctness of, so it will regress
  • it's unclear what to do when adding new files

Given the opportunity I'd prefer to optimise for making life easier for new contributors.

@mgreter
Copy link
Contributor Author

mgreter commented Oct 22, 2016

@xzyfer the order is not really important as long as all files are listed. If a file is missing it will fail in CI as functions will be missing in the executable. I don't see any danger at all here!

@mgreter
Copy link
Contributor Author

mgreter commented Oct 22, 2016

And it was as unclear where to add files before as it is now. The order is now even a bit more logic as I grouped the related stuff together where possible. I just needed to move the bigger files more to the top, so I don't end up with one as the last compile unit.

@xzyfer
Copy link
Contributor

xzyfer commented Oct 22, 2016

Currently they're listed alphabetically so it is no brainer where a new file belongs in the list.

I'm happy to comprise. Maintain alphabetical ordering, but move the problematic large files to the top, preferably with a comment for why it's out of order.

This should give slightly faster build times when
compiling in parallel with many threads.

bigger files (in terms of compile time) tend to go to the top,
so they don't end up as the last compile unit when compiling
in parallel. But we also want to mix them a little too avoid
heavy RAM usage peaks. Other than that the order is arbitrary.
- Fixes parent selector mappings
- Fixes media block/query mappings
- Fixes variable assignment mappings
- Fixes range over binary expressions
- Don't include semicolon for statics
@mgreter mgreter force-pushed the bugfix/source-mappings branch from 51ff53f to de83d30 Compare October 22, 2016 03:25
@mgreter
Copy link
Contributor Author

mgreter commented Oct 22, 2016

I'm ok with adding a comment, but the order is the best as it is now. It is a compromise between having the biggest compile units on top without generating a too heavy memory peak (it's distributed more evenly).

@xzyfer
Copy link
Contributor

xzyfer commented Oct 22, 2016

This is good. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants