-
Notifications
You must be signed in to change notification settings - Fork 157
fix: Sass partials imports from node_modules #256
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
Sass supports importing partials with or without the leading underscore: https://sass-lang.com/documentation/at-rules/import#partials Because `applyModuleNameMapper()` uses `require.resolve()` to employ node module resolution on a bare import, it throws an exception if the import elides the leading `_` from the basename.
@lmiller1990 should I be targeting the |
@vvanpo I will review this soon. Are you wanting to use this with Vue 2 or Vue 3? I think it's fine to target whichever you like then we can back (or forward) port it - we will need to maintain vue-jest 4.0 (for Vue 2) for quite some time, I expect. It's ideal to have the same feature set across both 4.0 and 5.0, though. |
No problem, take your time. Since it's a fix for a bug that exists in both |
So I hit another bug, where any import like I can think of even more edge-cases, though. Like what about bare imports resolving to Writing our own resolver is kind of a headache because we have to mirror Sass' exact implementation. The only reason we need our own resolver is to support |
Ok, I think this code seems fine - are you happy to have it merged as-is? (and then think more about the edge cases you explained above) I do have some questions:
|
For the vast majority of tests, no. But I have come across tests that run assertions against
Yes, this primarily.
The files themselves do, but not the imports! It's common to elide the file extension when importing. In fact, when importing I'm happy have this merged as-is, once you do, I'll open the other PR. Thank you! |
I will release this fix by the end of the week. I want to see if I can solve one more outstanding issue (#258) (not sure if this is a problem in 4.x or not but I'd like to find out). |
Sass supports importing partials with or without the leading underscore:
https://sass-lang.com/documentation/at-rules/import#partials
Because
applyModuleNameMapper()
usesrequire.resolve()
to employ node module resolution on a bare import, it would throw an exception if the import elided the leading_
from the basename. For example:Writing the test for this was a bit tricky, so I had to make some changes to the test runner. Node module resolution only ever uses realpaths, so when
module-name-mapper-helper.js
makes calls torequire.resolve()
, node won't be looking in the test directory'snode_modules
. As an aside, can we renamemodule-name-mapper-helper.js
to make it more clear that it is a Sass-specific importer? This changeset especially makes it specific to Sass. If that wasn't the intention with that module, then I suppose I could revert it and simply wrap the entirety ofapplyModuleNameMapper()
in atry ... catch
and call it twice.