sass/node-sass -> sass/dart-sass #295
Conversation
|
I wholeheartedly agree with swapping out |
| }; | ||
| }; |
There was a problem hiding this comment.
Avoid changing coding style in unrelated places if possible (even if the change of whitespace is correct). This adds noise to the PR and makes review harder.
There was a problem hiding this comment.
I'm moving it to a separate PR
| ] | ||
| ] |
There was a problem hiding this comment.
Same here. Avoid unnecessary changes.
There was a problem hiding this comment.
I'm moving it to a separate PR
| for (const possibleFile of possibleFiles) { | ||
| if (!possibleFile.startsWith('./') && !possibleFile.startsWith('{}') && !isAbsolute) { | ||
| possibleFiles.push('./' + possibleFile); | ||
| possibleFiles.push('{}/' + possibleFile); | ||
| } | ||
| } | ||
|
|
||
| for (const possibleFile of possibleFiles) { | ||
| if (possibleFile.startsWith('./') && !possibleFile.startsWith('{}') && !isAbsolute) { | ||
| possibleFiles.push('{}/' + possibleFile.substring('./'.length)); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This changes the possibleFiles array while looping over it. Avoid that and instead do something like this:
if (!isAbsolute) {
let additionalPossibleFiles = [];
for (const possibleFile of possibleFiles) {
if(...) {
additionalPossibleFiles.push(...)
}
}
possibleFiles = possibleFiles.concat(additionalPossibleFiles);
}Also I'm wondering why it is necessary to add those variants. Is this something required by dart-sass or is it an additional feature you like to include in meteor-scss package?
There was a problem hiding this comment.
This changes the possibleFiles
I'd gladly do it but I tried to go along the project code style. It's already done in a bunch of places. Should we go forward with immutability in only one place, leaving it mutable in older code? If not, shouldn't it be a matter of a separate pull request (probably, together with code formatting)?
There was a problem hiding this comment.
Is this something required by dart-sass or is it an additional feature you like to include in meteor-scss package?
Rechecked with my project, doesn't seem to be needed indeed.
| return { absolute: isAbsolute, path: possibleFile }; | ||
| return { absolute: isAbsolute, path: possibleFile }; |
There was a problem hiding this comment.
I'm moving it to a separate PR
| console.debug('imported style not found in possible files', possibleFiles); | ||
|
|
There was a problem hiding this comment.
Stray debug message. Please remove.
| let possibleFilePath, foundFile; | ||
| let possibleFilePath, foundFile, possibleFiles = []; | ||
|
|
||
| for (let includePath of _includePaths) { | ||
| possibleFilePath = path.join(includePath, importPath); | ||
| foundFile = getRealImportPathFn(possibleFilePath); | ||
| if (!importPath.match(/\.s?(a|c)ss$/)) { | ||
| for (const extension of expectedExtensions){ | ||
| possibleFiles.push(`${importPath}.${extension}`); | ||
| } | ||
| } else { | ||
| possibleFiles.push(importPath); | ||
| } | ||
|
|
||
| if (foundFile) { | ||
| return foundFile; | ||
| for (let includePath of _includePaths) { | ||
| for (let possibleFile of possibleFiles) { | ||
| possibleFilePath = path.join(includePath, possibleFile); | ||
| foundFile = getRealImportPathFn(possibleFilePath); | ||
| if (foundFile) { | ||
| return foundFile; | ||
| } |
There was a problem hiding this comment.
Same question here: Is this something required by dart-sass or is it an additional feature you like to include in meteor-scss package?
There was a problem hiding this comment.
Checked with my app, doesn't seem to be needed indeed.
| @@ -297,6 +323,7 @@ function _loadIncludePaths(config) { | |||
| } else { | |||
| _includePaths = []; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
I'm moving it into a separate PR
| return _getConfig('scss-config.json') || {}; | ||
| return _getConfig('scss-config.json') || { | ||
| }; |
dearlordylord
force-pushed
the
master
branch
from
f44357f to
af2f0aa
Compare
|
Thank you for feedback @znerol . A new PR coming soon. |
To overcome the issue #294 we can switch to https://www.npmjs.com/package/sass .
This code works quite well for my project. Our css person uses :not selector extensively so it's not viable to change codebase.
Also, the original node-sass issue (sass/node-sass#2330) seems to exist for a couple years already, therefore it's not clear when and if it will be resolved.