-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
Output CSS in /static/css
and switch to using stylus directly
#2544
Conversation
@nodejs/website This could use reviews. |
const labelForBuild = '[metalsmith] build/layouts finished' | ||
function buildCSS () { | ||
console.log('[stylus] static/css started') | ||
const labelForBuild = '[stylus] static/css finished' | ||
console.time(labelForBuild) | ||
|
||
fs.mkdir(path.join(__dirname, 'build'), () => { |
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.
Did you consider the { recursive: true }
option of fs.mkdir()
to avoid the nested callbacks below? As described by docs;
// Creates /tmp/a/apple, regardless of whether `/tmp` and /tmp/a exist.
fs.mkdir('/tmp/a/apple', { recursive: true }, (err) => {
if (err) throw err;
});
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.
Yeah, but I thought this change should be made separately after all the other changes have settled :)
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.
Also, this requires Node.js 10, another reason I didn't make this change yet.
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.
Is that requirement a problem though?
As far as I see it, we're running Node.js 10 in CI and $latest when building on the hosting server nodejs/build/setup/www/resources/scripts/build-site.sh.
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 problem for me, it's just that such change should be a clear patch IMO and not sneaked in in other patches, that is all.
Really cool to get rid of seemingly unnecessary abstraction to use stylus 💯 |
Draft because it requires #2461 and #2462.Fixes #2467