-
Notifications
You must be signed in to change notification settings - Fork 370
Fix crash when @importing a Sass file that @uses another file #2599
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
Conversation
|
Please fix the CI |
|
I believe sass-parser is broken at |
d521d43 to
320dd58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make the commit message shorter. The Linux git style guide recommends 50 characters for the first line of a commit.
lib/src/visitor/async_evaluate.dart
Outdated
| _importParent = _parent; | ||
| _parent = _root; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the issue here is that _parent is reset to _root. What happens if we don't do that, rather than adding an extra variable to track (and then need to un-track) a different type of parent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this is used to track ordering when doing @import containing @use.
e.g.
// This file is @imported into main.scss
@use 'uptream'
.foo-midstream{}Results in
.foo-midstream{}
.foo-upstream{}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the existence of _parent cause that?
320dd58 to
af92d93
Compare
|
I updated the tests but I'm still not clear whether |
discussed offline, it should fail regardless. It is only I'll also try to figure out why was |
7cdf182 to
7c9aa14
Compare
7c9aa14 to
33b0338
Compare
|
I think this is now ready, the crash actually went away recently. The Sass compiler still fails but the crash was less crashy :) |
Co-authored-by: Natalie Weizenbaum <[email protected]>
The crash happened when
@importinga Sass file that@usesa user-defined module, only when the Sass file@includesa mixin at the root level if the mixin emits a declaration that would end up emitted at the root. This is allowed because the@importwithin a style rule would end up causing that top-level declaration to be wrapped by the style rule, and therefore won't be a top-level declaration. However, this crashed the compiler instead.Fixes: #2588
Specs: sass/sass-spec#2060