Node module resolution for CSS import

[Alasdair McLeay (penx)]

Add 'Go to definition' support for

@import "~bootstrap/dist/css/bootstrap";

as per

• https://github.com/webpack-contrib/sass-loader#imports
• https://github.com/webpack-contrib/css-loader#import

Fixes microsoft/vscode-css-languageservice#136

Also see https://stackoverflow.com/questions/48499853/how-to-jump-to-definition-for-a-file-in-node-modules-from-vs-code

[Microsoft Contribution License Agreements (msftclas)]

All CLA requirements met.

[Alasdair McLeay (penx)]

I'm not sure how this can be unit tested, I guess I'd need to mock require.resolve, I don't see an equivalent test for
https://github.com/Microsoft/vscode-languageserver-node/blob/master/server/src/resolve.ts

[Alasdair McLeay (penx)]

I could add unit tests for this fairly easily, but that would lead me to put it in its own module, which in turn makes me wonder if there's an existing util in vscode that already does this that I could use instead?

[Alasdair McLeay (penx)]

the unit tests for this part of the code (completion.test.ts) only seem to be testing the public API rather than internal functions so I'm not going to add unit tests for now. Happy to take direction on this if you want unit tests adding!

[Alasdair McLeay (penx)]

Pine (@octref) previous build issues were not related to this PR. All solved now after rebase off master.

Please let me know if you'd like any changes

[Pine (octref)]

You are also in this thread: sass/sass#2350.

I'm not sure if VS Code should add such url resolving mechanism. It's not part of css or sass language, and only comes from some webpack plugins that you have configured. Ping Martin Aeschlimann (@aeschli)

[Alasdair McLeay (penx)]

some webpack plugins that you have configured

sass-loader and css-loader are the standard webpack loaders for styling and the behaviour this PR supports is for their default behaviour without any additional configuration.

https://webpack.js.org/loaders#styling

These loaders are used by many config-less uses of webpack, e.g. when using create-react-app

I agree that this isn't in the underlying spec for Sass, however I suspect that a very large number of people using VSCode to write Sass are using sass-loader under the hood. Even more so, I think it's safe to assume that near to 100% of vscode users that encounter @import "~something"; in a Sass file would be expecting this to be using node module resolution when compiled.

If you're still against this being the default behaviour of "Go to definition" in VSCode then perhaps it can be behind a config option instead? I personally would find this very useful, and I use what I believe are fairly standard build setups for writing CSS/Sass.

[Mina Fawzy (minkir014)]

Can't you provide autocompletion also??

[Alasdair McLeay (penx)]

Waiting for Martin Aeschlimann (@aeschli) to confirm this feature can be merged first!

[Mina Fawzy (minkir014)]

Alasdair McLeay (@penx) This feature is needed too much. if you can do that please post another PR.

[Martin Aeschlimann (aeschli)]

Alasdair McLeay (@penx) Thanks, sorry for the delay!

[Martin Aeschlimann (aeschli)]

Tests would be very appreciated. I created a first link tests: ae6be63
Would be great if you can add one for your feature.

To run, cd extensions/css-language-features && yarn test

[Mina Fawzy (minkir014)]

Alasdair McLeay (@penx) This PR has finally been merged. Please start another PR that provides variables completion across files.

[Martin Aeschlimann (aeschli)]

I had to rewrite the code as we can't use require: The code is webpackaged and webpack is very against require. See 8f72934
Alasdair McLeay (@penx) You want to look at my changes and fix it if I misunderstood how this works.

[Alasdair McLeay (penx)]

Martin Aeschlimann (@aeschli) sorry for the delay in responding, yes unfortunately what you have written assumes that join(documentFolder, 'node_modules', _moduleName, 'package.json'); will resolve the correct location on disk, but this will not work in many cases. e.g. nested dependencies, npm link, peerDependencies, yarn PnP, yarn portal.

See:

• https://twitter.com/arcanis/status/1052305689886875648

require is exactly what we want here as we want to know where node resolves the module to on disk.

Is it possible that we do something like if(require) or if(webpacked) and use require only in safe environments?

[Martin Aeschlimann (aeschli)]

I you find a way to trick webpack, I'm happy to take a PR.

[Alasdair McLeay (penx)]

Martin Aeschlimann (@aeschli) ok I should be able to look in to this, is there a way I can replicate the issue? Is there a particular build task I need to run to get the webpacked version that was causing the issue?

[Alasdair McLeay (penx)]

From what I understand, the development build I was using is not webpacked, but when you do a production/distribution build it is.

If I run yarn gulp -- vscode-darwin I guess this should produce a Mac OS X application that I can test, though on my branch this results in

Error: ModuleDependencyWarning: Critical dependency: require function is used in a way in which dependencies cannot be statically extracted

Which I guess is the issue you encountered when you merged my PR.

[Martin Aeschlimann (aeschli)]

You can manually invoke webpack by calling
npx webpack-cli --config=extension.webpack.config.js
in extensions/css-language-features

[Alasdair McLeay (penx)]

I'm still thinking about this but have had limited time. From some initial tests I think the current implementation only works if you're editing an scss file that's in the same directory as the node_modules folder you want to jump in to, which is rarely the case.