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

sass-loader 12 no longer compatible with node-sass-import-once #972

Closed
ThiefMaster opened this issue Jun 23, 2021 · 10 comments
Closed

sass-loader 12 no longer compatible with node-sass-import-once #972

ThiefMaster opened this issue Jun 23, 2021 · 10 comments

Comments

@ThiefMaster
Copy link

ThiefMaster commented Jun 23, 2021

  • Operating System: Linux
  • NPM Version: 14.16.1
  • Node Version: 7.18.1
  • webpack Version: 5.39.1
  • sass-loader Version: 12.1.0

Expected Behavior

https://github.com/at-import/node-sass-import-once working fine to de-duplicate imported css rules

Actual Behavior

It doesn't work, I get many duplicate rules in my final css file, since my (rather large) scss has some imports repeated in multiple files (usually to get some mixins etc, but we have both css rules and mixin in some of those files)


I do not have a minimal example yet, so I'm hoping it's something obvious, e.g. some API change that broke this (very old) library.

@alexander-akait
Copy link
Member

Please open an issue in node-sass-import-once, also it is unsafe to de-duplicate imported css rules

@alexander-akait
Copy link
Member

alexander-akait commented Jun 23, 2021

Also we don't change API for v12, only drop old Node.js version and fix small bug with .index. files for sass (dart-sass)

@ThiefMaster
Copy link
Author

Please open an issue in node-sass-import-once`

Last activity 5 years ago, so I doubt that'll do any good.

also it is unsafe to de-duplicate imported css rules

But usually it works fine.... With many different CSS files and modules in a larger application it is not feasible to rewrite the scss to remove the duplicates without a massive amount of work.

@alexander-akait
Copy link
Member

alexander-akait commented Jun 23, 2021

If you really need help please - create reproducible example (you can just create minimal example), without code I can't say what is wrong and this problems will be never resolved.

some API change that broke this (very old) library is really not help to find the problem place, we have a lot of places with complex logic

@ThiefMaster
Copy link
Author

Sure, I hoped you might have a idea what could have caused this :) I'll try to debug it a bit myself and then create a minimal testcase to reproduce it.

@alexander-akait
Copy link
Member

@ThiefMaster feel free to ping me, note - by default we use sass (dart-sass), but look like node-sass-import-once for node-sass, you can start with this

@ThiefMaster
Copy link
Author

ThiefMaster commented Jun 23, 2021

I haven't been able to reproduce it in a minimal example yet (would have been too easy I guess...), but I did manage to track down the problem: It's in #958: node-sass-import-once stores the cache on this but with the change in #958 that no longer works, so it always sees a new cache.

@ThiefMaster
Copy link
Author

@alexander-akait here's a minimal testcase to reproduce it: https://github.com/ThiefMaster/sass-loader-test

@ThiefMaster
Copy link
Author

I managed to work around the issue using this wrapper:

function _importOnce(...args) {
  // Due to https://github.com/webpack-contrib/sass-loader/pull/958/files we need to
  // use a different context for the importOnce plugin in order to keep the cache
  // between calls to this function.
  // Using a global one might work as well but like this it's scoped with the same
  // lifetime which we had before the sass-loader update as `options` is cloned at
  // the same time the object previously used in `this` was created (`context` in
  // `getOptions`).
  this.options._ctx = this.options._ctx || {options: this.options};
  return importOnce.apply(this.options._ctx, args);
}

ThiefMaster added a commit to indico/indico that referenced this issue Jun 23, 2021
@alexander-akait
Copy link
Member

@ThiefMaster I see... but really on this, is not safe due multi threading, ideally cache should be stored in independent variable/file

plourenco pushed a commit to plourenco/indico that referenced this issue Jul 6, 2021
pferreir pushed a commit to pferreir/indico that referenced this issue Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants