-
Notifications
You must be signed in to change notification settings - Fork 915
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
add plugin-sass with new onChange/markChanged interface #1225
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/lcb1g2skj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 🎉
5e92fe2
to
7ec6e90
Compare
plugins/plugin-sass/plugin.js
Outdated
.filter((s) => s.trim()) | ||
.map((s) => { | ||
if (!path.extname(s)) { | ||
s += '.scss'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might need to try and resolve .sass
files as a 2nd try?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think I came up with a good shortcut, lmk what you think
7ec6e90
to
2227e5d
Compare
2227e5d
to
dcc5d99
Compare
@drwpow ready for another look |
|
||
## Plugin API Methods | ||
|
||
### this.markChanged() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this idea
|
||
### onChange() | ||
|
||
Get notified any time a watched file changes. This can be useful when paired with the `markChanged()` plugin method, to mark multiple files changed at once. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for clarity, what constitutes a “watched file” from the plugin’s perspective?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basically, any file that Snowpack detects as "changed" will hit this. Open to any ideas that make this more clear!
|
||
Sass is interesting in that multiple compilers are available: [sass](https://www.npmjs.com/package/sass) (written in Dart) & [node-sass](https://www.npmjs.com/package/node-sass) (written in JavaScript). Both packages run on Node.js and both are popular on npm. However, [node-sass is now considered deprecated](https://github.com/sass/node-sass/issues/2952). | ||
|
||
**This plugin was designed to work with the `sass` package.** `sass` is automatically installed with this plugin as a direct dependency, so no extra effort is required on your part. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sidenote: maybe we should set up Dependabot for Snowpack, at least to prompt version pushes? While being always up-to-date isn’t possible, we should keep this reasonably up-to-date, especially with all the recent changes/improvements to Sass (it’s had a lot of development this year again, after a period of stagnation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're actually really lax about which version of Sass that we take here, so it would take a full major Sass update for us to get out of date. But yea, I'm not opposed to trying out a very quiet, major-only dependabot setup 👍
|
||
.App { | ||
text-align: center; | ||
background: base.$primary-color; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No semicolons in .sass
files (they’re pure indentation, like YAML)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! this actually needed a small fix in the plugin 👍
12f303d
to
ae63815
Compare
Changes
onChanged()
hook for plugins to listen for changed files during development.markChanged()
method that plugins can call to mark a file as changed.Testing
TODOTests added.Docs
TODO:Docs added to README, plugins guide, and sass recipe