-
Notifications
You must be signed in to change notification settings - Fork 366
Fix a bug where a double-configuration error wouldn't be emitted #2600
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
0cf97b8
to
1b3c2f9
Compare
Thank you for the update @nex3: this is great! It appears to resolve the examples we've discussed and to accomplish what I intended in PR #2602. There is a minor issue if the variable is in an // App.scss (top-level Sass file)
@use 'scss/coreui2' with (
$white: #aaa
) ; // scss/coreui2.scss
@use "variables";
@forward "functions";
with the remaining files are as described in Issue #2598, and in https://github.com/arikorn/sass-bug-demo The result is:
The error message is wrong ($white is declared with Just to be clear: the only change here from the previous examples is that coreui2.scss loads variables.scss with Note, though, that this is not a breaking change, since the same input causes an error when using the main branch as well. I have other specific comments that I'll attach to the code review, but nothing bigger than this. |
Oh drat! It also fails the test you mention here, which can be rephrased as: // App.scss (top-level Sass file)
@use 'scss/coreui2' as _;
@use 'scss/coreui2' with (
$white: #aaa
) ; (Giving a "variable not declared with !default" error instead of "module was already loaded" error.) Perhaps the answer is that this is a work-in-progress, and those unused functions will fix it? |
This error message is correct.
Good catch. Looks like we're not handling "could have been configured" logic correctly for forwards. |
This didn't account for forwarded modules when determining which variables were configurable in a given module.
Closes #2598
See sass/sass#4111
See sass/sass-spec#2061