Skip to content
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

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

Conversation

penx
Copy link
Contributor

@penx penx commented Aug 22, 2019

Further to #70693 which was reworked by @aeschli in 8f72934 due to issues with require.resolve and webpack.

Fixes #78894
Fixes microsoft/vscode-css-languageservice#136

The current implementation of resolving the path to a CSS module in node_modules has significant limitations.

Using require.resolve directly should be the most solid approach but can't be used in code that is webpacked.

This PR allows us to use require.resolve via a module proxy (requireresolveproxy) and marking the module as a commonjs external in the webpack config.

Potential alternative approaches:

Screenshot 2019-08-23 at 16 18 58
Screenshot 2019-08-23 at 16 19 48

@penx penx changed the title use require.resolve to find CSS modules in node_modules Use require.resolve to find CSS modules in node_modules Aug 22, 2019
@aeschli aeschli requested a review from octref August 23, 2019 06:57
@penx
Copy link
Contributor Author

penx commented Aug 23, 2019

This builds fine now when I run yarn gulp -- vscode-darwin, however I'm not sure how I run the production build on MacOS in order to test it. I have tried cd out-build && node main.js but get Error: Cannot find module 'electron'.

penx added 2 commits August 23, 2019 15:52
use require.resolve via a module proxy and mark it as a commonjs external in webpack config
@penx penx force-pushed the feature/node-module-resolution-for-css-import-requireresolveproxy branch from a1811cd to 7114b57 Compare August 23, 2019 14:54
@penx
Copy link
Contributor Author

penx commented Aug 23, 2019

Confirmed this is working in the dev build (yarn watch and ./scripts/code.sh) so taking out of draft - I can test in the production build too if someone lets me know how!

@penx penx marked this pull request as ready for review August 23, 2019 15:21
import { existsSync } from 'fs';

const resolve = require('requireresolveproxy');
Copy link
Contributor Author

@penx penx Aug 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is using require rather than import due to not having .ts typings for requireresolveproxy - happy to resolve in another way if someone can suggest how

@penx
Copy link
Contributor Author

penx commented Aug 27, 2019

Pretty sure the failing CI is unrelated to this PR

  49 passing (2s)
  1 failing

  1) Links node module resolving:

      AssertionError [ERR_ASSERTION]: 'file:///d%3A/a/1/s/extensions/css-language-features/server/test/linksTestFixtures/foo/hello.html' == 'file:///d%3A/a/1/s/extensions/css-language-features/server/test/linksTestFixtures/node_modules/foo/hello.html'
      + expected - actual

      -file:///d%3A/a/1/s/extensions/css-language-features/server/test/linksTestFixtures/foo/hello.html
      +file:///d%3A/a/1/s/extensions/css-language-features/server/test/linksTestFixtures/node_modules/foo/hello.html
      
      at assertLink (D:\a\1\s\extensions\css-language-features\server\out\test\links.test.js:23:16)
      at assertLinks (D:\a\1\s\extensions\css-language-features\server\out\test\links.test.js:37:13)

@aeschli
Copy link
Contributor

aeschli commented Aug 28, 2019

That's the test that I added for the node module resolving.

@penx
Copy link
Contributor Author

penx commented Sep 27, 2019

Replaced by #81555

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants