SCSS: resolve bare module imports on Windows (#245579)#440
SCSS: resolve bare module imports on Windows (#245579)#440IsaacTian05 wants to merge 9 commits intomicrosoft:mainfrom
Conversation
|
@microsoft-github-policy-service agree |
.travis.yml
Outdated
| @@ -0,0 +1,30 @@ | |||
| # Continuous Integration for vscode-css-languageservice | |||
| const doc = TextDocument.create('file:///c:/proj/app.scss', 'scss', 1, | ||
| "@import 'bootstrap/scss/variables';"); | ||
| const links = await ls.findDocumentLinks2(doc, ls.parseStylesheet(doc), {}, mockFS); | ||
| const target = links[0].target!.replace(/\\/g, '/'); |
There was a problem hiding this comment.
target is a URI, it should not contain \
aeschli
left a comment
aeschli
left a comment
There was a problem hiding this comment.
Looks good. If you can add some more test cases that would be great
src/services/cssNavigation.ts
Outdated
| // Treat bare module names (“bootstrap/...”) like sass-loader does | ||
| const isBareImport = !target.startsWith('.') // not ./ or ../ | ||
| && !target.startsWith('/') // not workspace-absolute | ||
| && target.indexOf(':') === -1; // not a scheme (file://) |
There was a problem hiding this comment.
Maybe a use a regex to make that check [
startsWithSchemeRegex = /^\w[\w\d+.-]:/
…solute Adds `schemeImportLinks.test.ts` with four mocha cases covering `http://`, `https://`, `file://`, and `vscode-resource://` URLs
|
|
||
| const ref = await this.mapReference(documentContext.resolveReference(target, documentUri), isRawLink); | ||
| // Treat bare module names (“bootstrap/...”) like sass-loader does | ||
| const startsWithSchemeRegex = /^\w[\w\d+.-]:/; |
There was a problem hiding this comment.
Node module resolving should only happen when this.resolveModuleReferences is true.
(only set for LESS and SCSS).
So this needs to go behind the if (this.resolveModuleReferences) {.
I looks like we already call resolveModuleReference there, so this code is not necessary?
| @@ -0,0 +1,46 @@ | |||
| import * as assert from 'assert'; | |||
There was a problem hiding this comment.
please move these into scssNavigation.test.ts, where we have all other tests for node module resolving.
Please use the already existing test framework and style (mocha with suite and test
| @@ -0,0 +1,18 @@ | |||
| import { assert } from 'chai'; | |||
There was a problem hiding this comment.
please move these into scssNavigation.test.ts, where we have all other tests for node module resolving.
Please use the already existing test framework and style (mocha with suite and test
|
If these changes are still pending, I can go ahead and implement them. ...Actually, how would I go about adding changes to someone else's pull request? Edit: Oh, I can't unless I have write access to the repo from which the pull request was made. That's incredibly annoying. Edit 2: Hopefully making a pull request to @IsaacTian05's fork allows my changes to in turn be added to this PR. |
3 participants
Fix issue #432