feat: pass loaderContext to custom importer

[alexander-akait]

In some case i need use addDependency or addContextDependency (or other useful api stuff), be great have ability to this. If you accept this i will write tests for this, thank you!

[codecov]

Codecov Report

Merging #513 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #513      +/-   ##
==========================================
+ Coverage   97.43%   97.45%   +0.02%     
==========================================
  Files           6        6              
  Lines         117      118       +1     
==========================================
+ Hits          114      115       +1     
  Misses          3        3

Impacted Files	Coverage Δ
lib/proxyCustomImporters.js	100% <100%> (ø)	⬆️
lib/normalizeOptions.js	96.42% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88b56d5...435ebfd. Read the comment docs.

[jhnns]

Hey @evilebottnawi, thank you for your PR 👍

Can you tell me a little bit more about the use-case of accessing the loader context in a custom importer? Usually you don't have to add files via addDependency by yourself since the sass-loader will already add all dependencies.

Besides that, I don't think that we should expose the loaderContext via this. this can be abused to pass an infinite number of function arguments and we don't really know what this we're actually extending since it is provided by node-sass. If we need to pass the loaderContext to a custom importer, I'd favor explicit function arguments.

[alexander-akait]

@jhnns use case - generate scss file dynamicly, example i need generate scss file(s) based on .env files and i want to use addDependency for regenerate scss after change .env file(s).

[alexander-akait]

@jhnns i rewrite this using arguments after we accept this 😄

[michael-ciniawsky]

In case this is lands resourcePath && loaderContext should be merged here, since resourcePath is already on the loaderContext

[jhnns]

There was almost the same feature request at the less-loader. Does the approach with nodemon I mentioned here work for you as well?

[alexander-akait]

@jhnns need tests again it is old PR 😄 Works as expected. Only one note, resourcePath already in loaderContext. Better pass only loaderContext, but it is breaking changes 😕

[bencergazda]

This would also make possible to use for example this.resolve() in an importer, which would help a lot when creating custom importers.

Are you planning to add this feature?

[alexander-akait]

/cc @jhnns

[bencergazda]

@jhnns @michael-ciniawsky Really just a friendly ping, are you planning to add this feature? Is there anything I can contribute to get it merged?

[alexander-akait]

/cc @jhnns @michael-ciniawsky

[michael-ciniawsky]

I think this is needed to some point and gives users the flexibility they often want and need, ideally it would be the passed as the second argument and passing the resourcePath separately is being removed, but that's a breaking change so maybe better to do it in a follow up PR

[codecov]

Codecov Report

Merging #513 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #513      +/-   ##
==========================================
+ Coverage   97.43%   97.45%   +0.02%     
==========================================
  Files           6        6              
  Lines         117      118       +1     
==========================================
+ Hits          114      115       +1     
  Misses          3        3              

Impacted Files	Coverage Δ
lib/normalizeOptions.js	96.42% <100.00%> (ø)
lib/proxyCustomImporters.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88b56d5...435ebfd. Read the comment docs.

[jhnns]

ideally it would be the passed as the second argument and passing the resourcePath separately is being removed, but that's a breaking change

proxyCustomImporters() is just an internal function. We can change the signature as we like.

I'm still not convinced to add this feature. The custom importer API belongs to Sass and it doesn't feel right to extend it.

A custom importer should not read several files to generate the source code. Ideally, it would just convert the given source code into Sass compatible code.

Can you explain me your use-case?

@evilebottnawi You've mentioned the .env files. Why don't you just import the .env file with Sass and then use a custom importer to translate it into Sass code?

@bencergazda Why do you need to use .resolve()? You can already alias Sass imports via webpack alias.

[bencergazda]

@jhnns I have a custom importer, which works as a part of a simple Sass/JS plugin-system – the importer checks all the candidate plugin paths and resolves with the array of the available file paths. It seems ideal to check the file path(s) with webpack's resolve algorithm, just how it would be done otherwise in case of a normal @import.

I have also tried webpack's enhanced-resolve before, but this led to strange errors in combination with node-sass (most probably threadpool issues, as a custom UV_THREADPOOL_SIZE setting solved it).

Using the loader's .resolve() looks to be a more elegant (and functional out-of-the-box) way.

(It was anyway an interesting discovery that I could get rid of the threadpool issue by using the .resolve() provided by the loader context, instead of using fs.exists() or creating a new resolver instance of enhanced-resolve)

[jhnns]

@bencergazda Your use case should also be solvable via webpack's resolver (either via the resolve options in your webpack.config.js or as plugin). The sass-loader uses webpack's resolve to resolve dependencies. Have you tried that approach?

[matt-curtis]

Maybe I've missed it in the course of the conversation, but was there ever a solution mentioned for indicating to webpack that a certain file should be watched? I have a custom sass importer that's able to import svg files as sass modules, which works fine — the problem is that I can't indicate to webpack that the svg file is one that it should watch, and, if it changes, rebuild the entire config.

From what I know, the addDependency method webpack loaders have access to is how this would usually be handled... but importers don't have access to it.

@jhnns Would love this feature added, or if not, some other kind of workaround. Should I just try to write a regular webpack loader?

[alexander-akait]

@matt-curtis Let's be solved for the next release, ETA is the next week, right now, you can write manual loader if you need this ASAP

[matt-curtis]

@matt-curtis Let's be solved for the next release, ETA is the next week, right now, you can write manual loader if you need this ASAP

Looking forward to it. Thanks!