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

Allow @forward multiple times without with () #1716

Closed
KaelWD opened this issue Jun 11, 2022 · 7 comments · Fixed by #1739
Closed

Allow @forward multiple times without with () #1716

KaelWD opened this issue Jun 11, 2022 · 7 comments · Fixed by #1739
Assignees

Comments

@KaelWD
Copy link

KaelWD commented Jun 11, 2022

Reference: vuetifyjs/vuetify#14446

// main.scss
@use 'library' with (
  $var: 'value',
);

// library.scss
@forward 'component-a';
@forward 'component-b';

// component-a.scss
@forward './variables';

// component-b.scss
@forward './variables';

// _variables.scss
$var: 'default' !default;

with is only used in main.scss, so the second forward can just be ignored instead of throwing

Error: This module was already loaded, so it can't be configured using "with".
  ┌──> component-b.scss
1 │   @forward './variables'
  │   ^^^^^^^^^^^^^^^^^^^^^^ new load
  ╵
  ┌──> component-a.scss
1 │   @forward './variables'
  │   ━━━━━━━━━━━━━━━━━━━━━━ original load
  ╵
  ┌──> main.scss
1 │ ┌ @use 'library' with (
2 │ │   $var: 'value',
3 │ │ );
  │ └─^ configuration
  ╵
  component-b.scss 1:1  @forward
  library.scss 2:1      @use
  main.scss 1:1         root stylesheet
@nex3
Copy link
Contributor

nex3 commented Jun 13, 2022

I can't reproduce this. What version of Sass are you using?

@Goodwine
Copy link
Member

If this is from vuetify, they seem to be using 1.32.13, I did a quick test and found that this exact code sample does indeed fail at that 1-year-old version and it actually fails for 1.33.0 too.

However this code snippet does not fail anymore from 1.34.0 onwards

@KaelWD
Copy link
Author

KaelWD commented Jun 14, 2022

We're only using the new module system in v3 (next branch), v2 is stuck on sass 1.32 because the division changes aren't compatible with the way consumers were overriding variables (@import in prependData).

Here's a minimal reproduction of it with sass 1.52.3 and no other libraries: https://github.com/KaelWD/sass-forward-twice
Seems to require the second variable that isn't defined in the first file, if you comment out main.scss:3 and _index.scss:4-5 it doesn't error.
Trying to override a non-existent variable with this setup also throws This module was already loaded instead of the expected This variable was not declared with !default in the @used module

@Goodwine
Copy link
Member

Goodwine commented Jun 22, 2022

I can repro, in this case I don't even need the tabs directory and it triggers with just the list directory if you have >=2 variables but not with just one 🤔

@Goodwine
Copy link
Member

After debugging and discussing with @nex3 we found this behavior to be "working as specified" but we agreed the desired behavior from this usecase is correct and we'll first need to change the spec, and then implement the fix

I'll mark this as blocked for now until that change in spec happens

@Goodwine Goodwine added the blocked Waiting on another issue to be fixed label Jul 13, 2022
@Goodwine
Copy link
Member

@KaelWD this should fix it, at least on your provided repro repo it does
#1739

It's still pending the spec changes being merged before we can make this change, but not sure if this would ultimately work for your real-world use-case, I think it should tho 🤔

(I verified with:

  • clone dart-sass
  • in dart-sass dart pub get && dart run grinder before-test
  • update package.json to "sass": "file:/path/to/dart-sass/build/npm" in your project
  • in your project npm install && npm run build

)
If you want to test feel free and you can let me know if you have any problems 🙂

Goodwine added a commit that referenced this issue Aug 18, 2022
* Allow a module loaded multiple times by the same configuration

* use object references as opaque ID

Fixes #1716
@Goodwine Goodwine removed the blocked Waiting on another issue to be fixed label Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants