Skip to content

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

Merged
merged 1 commit into from
Jul 29, 2020

Conversation

vvanpo
Copy link
Contributor

@vvanpo vvanpo commented Jul 27, 2020

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 would throw an exception if the import elided the leading _ from the basename. For example:

<style lang="scss">
@import "~foo/bar"; // Actual filepath might be `node_modules/foo/_bar.scss`.
</style>

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 to require.resolve(), node won't be looking in the test directory's node_modules. As an aside, can we rename module-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 of applyModuleNameMapper() in a try ... catch and call it twice.

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.
@vvanpo
Copy link
Contributor Author

vvanpo commented Jul 27, 2020

@lmiller1990 should I be targeting the next branch so that we backport this fix onto master after it's merged, or is it fine to target master and require the fix to be cherry-picked onto next?

@lmiller1990
Copy link
Member

@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.

@vvanpo
Copy link
Contributor Author

vvanpo commented Jul 28, 2020

No problem, take your time.

Since it's a fix for a bug that exists in both master and next, I think we'll want it merged into both branches. The project I'm currently experiencing this bug in is using Vue 2, but after Vue 3 is stable for a while I'll be upgrading so I'll need it fixed in vue-jest@5 as well, as I imagine others will too (this bug doesn't appear to exist in vue-jest v3).

@vvanpo
Copy link
Contributor Author

vvanpo commented Jul 28, 2020

So I hit another bug, where any import like @import '~foo.bar'; breaks, because our custom resolver thinks .bar is a file extension. I've fixed this in another branch, but it relies on the changes from this one so I'll open a PR once this one is merged: vvanpo/vue-jest@fix-module-partial...vvanpo:fix-module-dots-in-filename

I can think of even more edge-cases, though. Like what about bare imports resolving to _index.scss? That breaks too (just tested it), and I can already think of another project at my work that makes heavy use of that.

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 moduleNameMapper, and bare imports. moduleNameMapper is likely mostly used for mimicking Webpack aliases, so our resolver is really just trying to emulate sass-loader. I wonder if it wouldn't be possible to reuse the custom importer defined by sass-loader? They've already solved all of the bugs we're bumping up against: https://github.com/webpack-contrib/sass-loader/blob/v9.0.2/src/utils.js#L270
Maybe if we ask nicely and are willing to do the work they might let us extract all the Webpack-specific stuff out and export a more generic importer that vue-jest is able to use directly.

@lmiller1990
Copy link
Member

lmiller1990 commented Jul 29, 2020

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:

  • what is the use case for something like @import '~foo.bar';? I thought sass files always (and had to) end in .sass (at least in my experience).
  • do your sass stylesheets somehow change the behavior of your tests? Or was this bug just a blocker because vue-jest would fail to find the relevant stylesheet(s) and error out?

@vvanpo
Copy link
Contributor Author

vvanpo commented Jul 29, 2020

@lmiller1990

do your sass stylesheets somehow change the behavior of your tests?

For the vast majority of tests, no. But I have come across tests that run assertions against window.getComputedStyle(element). These are of limited value, though, as I believe jsdom doesn't have great support for it, so I've only seen a handful of tests like this.
But if you set experimentalCSSCompile: false, I believe class names don't show up at all if you use CSS modules. So all snapshot tests are filled with class="", and that can be confusing to developers and obscure potential bugs.

was this bug just a blocker because vue-jest would fail to find the relevant stylesheet(s) and error out?

Yes, this primarily.

I thought sass files always (and had to) end in .sass

The files themselves do, but not the imports! It's common to elide the file extension when importing. In fact, when importing .css files, you have to elide the extension or Sass will transform it into a dynamic CSS import. So an @import 'foo.bar'; is equivalent to @import 'foo.bar.scss';, or @import 'foo.bar.sass';, or @import 'foo.bar.css';... depending on what actually exists on the filesystem. If they all exist, node-sass has one particular behaviour, dart-sass apparently errors out. At least, that's what I gather from reading sass-loader's code.

I'm happy have this merged as-is, once you do, I'll open the other PR. Thank you!

@lmiller1990 lmiller1990 merged commit a85122c into vuejs:master Jul 29, 2020
@lmiller1990
Copy link
Member

lmiller1990 commented Jul 29, 2020

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).

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

Successfully merging this pull request may close these issues.

2 participants