Fix Sass partial changes not triggering recompiles in Dev #2792
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/9hiQXqozkhZcJ6B4ipnkH32XD5Xr |
drwpow
reviewed
Mar 9, 2021
| @@ -3,14 +3,37 @@ const path = require('path'); | |||
| const execa = require('execa'); | |||
| const npmRunPath = require('npm-run-path'); | |||
|
|
|||
| const IMPORT_REGEX = /\@(use|import)\s*['"](.*?)['"]/g; | |||
| const IMPORT_REGEX = /\@(use|import|forward)\s*['"](.*?)['"]/g; | |||
drwpow
reviewed
Mar 9, 2021
| .map((s) => { | ||
| return path.resolve(path.dirname(filePath), s); | ||
| // Avoid node packages and core sass libraries. | ||
| .filter((s) => !s.includes('node_modules') && !s.includes('sass:')) |
There was a problem hiding this comment.
Nice check for Sass builtins
drwpow
reviewed
Mar 9, 2021
| @@ -1,2 +1,4 @@ | |||
| // folder/_index.sass | |||
| @use 'child-partial' | |||
drwpow
reviewed
Mar 9, 2021
| @@ -94,7 +143,7 @@ module.exports = function sassPlugin(_, {native, compilerOptions = {}} = {}) { | |||
| // During development, we need to track changes to Sass dependencies. | |||
| if (isDev) { | |||
| const sassImports = scanSassImports(contents, filePath, fileExt); | |||
| sassImports.forEach((imp) => addImportsToMap(filePath, imp)); | |||
| [...sassImports].forEach((imp) => addImportsToMap(filePath, imp)); | |||
There was a problem hiding this comment.
Is a new array ([... ]) needed here?
drwpow
reviewed
Mar 9, 2021
|
|
||
| // If it is a directory then look for an _index file. | ||
| try { | ||
| if (fs.lstatSync(pathName).isDirectory()) { |
drwpow
approved these changes
Mar 9, 2021
|
Just released in |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
The @snowpack/plugin-sass plugin does not correctly trigger sass recompiling when partials past the first level of imports are changed during
snowpack dev. Although Snowpack recognizes that the file has changed, it does not trigger the top file importing it as changed so nothing is recompiled.The
scanSassImports()function will now recursively find all the child imports. It will also reduce duplicates since it is possible that many partials are importing the same file like a_variables.scsspartial. I also added support for@forwardwhich is also used for importing partials https://sass-lang.com/documentation/at-rules/forward.Related Discussions:
#1717
#2668
Testing
I first updated the
marks a dependant as changed when an imported changes and isDev=truetest to look for a child of a child dependency. This proved the bug by causing the test to fail. I then wrote the code to allow the test to pass. I also manually tested in a large project with multiple levels of nesting.Docs
Bug fix only.