Node module resolution for CSS import #70693

penx · 2019-03-18T11:49:13Z

Add 'Go to definition' support for

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

as per

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

msftclas · 2019-03-18T12:10:22Z

All CLA requirements met.

extensions/css-language-features/server/src/utils/documentContext.ts

penx · 2019-03-18T12:14:47Z

extensions/css-language-features/server/src/utils/documentContext.ts

+			// and [sass-loader's](https://github.com/webpack-contrib/sass-loader#imports)
+			// convention, if an import path starts with ~ then use node module resolution
+			// *unless* it starts with "~/" as this refers to the user's home directory.
+			if (ref[0] === '~' && ref[1] !== '/') {


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

penx · 2019-03-18T12:16:45Z

extensions/css-language-features/server/src/utils/documentContext.ts

@@ -8,6 +8,14 @@ import { endsWith, startsWith } from '../utils/strings';
 import * as url from 'url';
 import { WorkspaceFolder } from 'vscode-languageserver';

+function getModuleNameFromPath(path: string) {


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?

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!

Add support for `@import "~bootstrap/dist/css/bootstrap";` as per https://github.com/webpack-contrib/sass-loader#imports Fixes microsoft/vscode-css-languageservice#136

extract resolvePathToModule and ensure we resolve to package root

if require.resolve fails, defer to default behaviour -url.resolve(base, ref)

penx · 2019-03-20T18:34:47Z

@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

octref · 2019-04-01T17:13:10Z

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

penx · 2019-04-10T12:41:54Z

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.

minkir014 · 2019-05-31T16:27:03Z

Can't you provide autocompletion also??

penx · 2019-05-31T17:43:43Z

Waiting for @aeschli to confirm this feature can be merged first!

minkir014 · 2019-05-31T18:58:13Z

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

aeschli · 2019-06-04T07:54:02Z

@penx Thanks, sorry for the delay!

aeschli · 2019-06-04T08:37:02Z

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

minkir014 · 2019-06-04T11:01:59Z

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

aeschli · 2019-06-05T11:07:13Z

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

penx · 2019-07-12T10:33:31Z

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

aeschli · 2019-07-12T10:53:36Z

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

penx · 2019-07-12T10:56:40Z

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

penx · 2019-07-12T13:21:37Z

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.

aeschli · 2019-07-12T17:59:27Z

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

penx · 2019-08-22T10:33:49Z

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.

penx mentioned this pull request Mar 18, 2019

Ctrl+click (go to file) doesn't work for saas-loader ~ imports (node_modules) microsoft/vscode-css-languageservice#136

Closed

penx commented Mar 18, 2019

View reviewed changes

extensions/css-language-features/server/src/utils/documentContext.ts Outdated Show resolved Hide resolved

penx commented Mar 18, 2019

View reviewed changes

isidorn assigned octref Mar 18, 2019

penx force-pushed the feature/node-module-resolution-for-css-import branch from 386069f to a2c0729 Compare March 19, 2019 14:31

Alasdair McLeay added 4 commits March 20, 2019 18:05

Node module resolution for CSS import

f0b9a60

Add support for `@import "~bootstrap/dist/css/bootstrap";` as per https://github.com/webpack-contrib/sass-loader#imports Fixes microsoft/vscode-css-languageservice#136

add some documentation

30b5469

resolve to package root

b170858

extract resolvePathToModule and ensure we resolve to package root

handle require.resolve failure

2e9bd07

if require.resolve fails, defer to default behaviour -url.resolve(base, ref)

penx force-pushed the feature/node-module-resolution-for-css-import branch from a2c0729 to 2e9bd07 Compare March 20, 2019 18:06

aeschli merged commit df9b668 into microsoft:master Jun 4, 2019

aeschli self-assigned this Jun 5, 2019

aeschli added this to the June 2019 milestone Jun 7, 2019

aeschli mentioned this pull request Jun 7, 2019

SCSS follow link resolves wrong path (prefixes base with underscore) #58204

Closed

Friksel mentioned this pull request Aug 11, 2019

[scss] link doesn't work with ~Tilde to search in node module or webpack alias #78894

Closed

penx mentioned this pull request Aug 22, 2019

Use require.resolve to find CSS modules in node_modules #79651

Closed

penx mentioned this pull request Sep 27, 2019

CSS - Search in parent folders for node module resolution #81555

Merged

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node module resolution for CSS import #70693

Node module resolution for CSS import #70693

penx commented Mar 18, 2019 •

edited

Loading

msftclas commented Mar 18, 2019 •

edited

Loading

penx Mar 18, 2019

penx Mar 18, 2019

penx Mar 19, 2019

penx commented Mar 20, 2019

octref commented Apr 1, 2019

penx commented Apr 10, 2019 •

edited

Loading

minkir014 commented May 31, 2019

penx commented May 31, 2019

minkir014 commented May 31, 2019

aeschli commented Jun 4, 2019

aeschli commented Jun 4, 2019 •

edited

Loading

minkir014 commented Jun 4, 2019

aeschli commented Jun 5, 2019

penx commented Jul 12, 2019 •

edited

Loading

aeschli commented Jul 12, 2019

penx commented Jul 12, 2019

penx commented Jul 12, 2019

aeschli commented Jul 12, 2019

penx commented Aug 22, 2019

Node module resolution for CSS import #70693

Node module resolution for CSS import #70693

Conversation

penx commented Mar 18, 2019 • edited Loading

msftclas commented Mar 18, 2019 • edited Loading

penx Mar 18, 2019

Choose a reason for hiding this comment

penx Mar 18, 2019

Choose a reason for hiding this comment

penx Mar 19, 2019

Choose a reason for hiding this comment

penx commented Mar 20, 2019

octref commented Apr 1, 2019

penx commented Apr 10, 2019 • edited Loading

minkir014 commented May 31, 2019

penx commented May 31, 2019

minkir014 commented May 31, 2019

aeschli commented Jun 4, 2019

aeschli commented Jun 4, 2019 • edited Loading

minkir014 commented Jun 4, 2019

aeschli commented Jun 5, 2019

penx commented Jul 12, 2019 • edited Loading

aeschli commented Jul 12, 2019

penx commented Jul 12, 2019

penx commented Jul 12, 2019

aeschli commented Jul 12, 2019

penx commented Aug 22, 2019

penx commented Mar 18, 2019 •

edited

Loading

msftclas commented Mar 18, 2019 •

edited

Loading

penx commented Apr 10, 2019 •

edited

Loading

aeschli commented Jun 4, 2019 •

edited

Loading

penx commented Jul 12, 2019 •

edited

Loading