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

Feat: support new resolving import at-rules #255

Merged
merged 12 commits into from
Dec 9, 2021

Conversation

fyangstudio
Copy link
Contributor

@fyangstudio fyangstudio commented Oct 27, 2021

https://github.com/webpack-contrib/sass-loader#resolving-import-at-rules

Using ~ is deprecated and can be removed from your code (we recommend it), but we still support it for historical reasons. Why can you remove it? The loader will first try to resolve @import as a relative path. If it cannot be resolved, then the loader will try to resolve @import inside node_modules.

Support VS Code DefinitionProvider find right package like:

@import "bootstrap";
@import "~bootstrap";

@fyangstudio
Copy link
Contributor Author

@aeschli

@aeschli
Copy link
Contributor

aeschli commented Nov 24, 2021

The loader will first try to resolve @import as a relative path. If it cannot be resolved, then the loader will try to resolve @import inside node_modules

The looking up of an import in node_modules, Is this something webpack does, or SCSS?

@fyangstudio
Copy link
Contributor Author

The loader will first try to resolve @import as a relative path. If it cannot be resolved, then the loader will try to resolve @import inside node_modules

The looking up of an import in node_modules, Is this something webpack does, or SCSS?

@aeschli Webpack and Sass are both mentioned that imports will always be resolved relative to the current file first, see:

Webpack: https://github.com/webpack-contrib/sass-loader#resolving-import-at-rules
Sass: https://sass-lang.com/documentation/at-rules/import#load-paths

This PR support node module resolution without ~.

PS: using ~ is work too

@aeschli
Copy link
Contributor

aeschli commented Nov 29, 2021

Thanks @fyangstudio.
I wonder if we should only enable this for SCSS. Right now, that change would enable the extended resolving also for LESS and CSS.
For CSS there's the css-loader. If I get this right, it still requires '~'.

Also, it would be nice to have tests as well.

src/services/cssNavigation.ts Outdated Show resolved Hide resolved
@fyangstudio
Copy link
Contributor Author

fyangstudio commented Nov 30, 2021

Thanks @fyangstudio. I wonder if we should only enable this for SCSS. Right now, that change would enable the extended resolving also for LESS and CSS. For CSS there's the css-loader. If I get this right, it still requires '~'.

Also, it would be nice to have tests as well.

'~' is deprecated in Less too, see https://github.com/webpack-contrib/less-loader#imports

I have changed it only enable new rule for Sass and Less.

@aeschli Please check.

By the way I think css-loader will support the new resolving import at-rules soon.

@fyangstudio
Copy link
Contributor Author

@aeschli if this PR have any questions, please let me know.

@aeschli
Copy link
Contributor

aeschli commented Dec 9, 2021

I polished the check whether to resolve module references (6e64249)

All looks good now, thanks @fyangstudio !

@aeschli aeschli merged commit 45016d3 into microsoft:main Dec 9, 2021
@aeschli aeschli added this to the January 2022 milestone Dec 9, 2021
@aeschli aeschli self-assigned this Dec 9, 2021
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