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 stylus support #1437

Merged
merged 10 commits into from Jul 12, 2017

Conversation

Projects
None yet
3 participants
@iansinnott
Member

iansinnott commented Jul 9, 2017

What changed

  • I filled out the gatsby-plugin-stylus module. We now have stylus support.
  • I added "stylus" as a whitelisted key in webpack-validator. This allows full support of the stylus-loader
  • I added what I hope is a helpfel error message for when the webpack validation fails

Fixes #1432

iansinnott added some commits Jul 9, 2017

Add jest-cli to dependencies
I was getting a module resolution error after installing. Test command was
throwing an error saying it couldn't find jest-cli so now its in deps.
Add a validation whitelist to allow the "stylus" key in webpack config
This is a special case but we may run in to other cases before upgrading to
webpack v3. In webpack v1 many plugins and loaders used top-level configuration
keys to handle custom configuration. webpack-validator does not like this, but
it does provide the option to whitelist keys, which is exactly what I did here.

I also added a (hopefully) helpful error message to clarify what went wrong in
the case of an invalid webpack config.

@iansinnott iansinnott changed the title from Add stylus support. Fixes #1432 to Add stylus support Jul 9, 2017

@KyleAMathews

This comment has been minimized.

Show comment
Hide comment
Contributor

KyleAMathews commented Jul 9, 2017

@@ -30,6 +30,7 @@
"iflow-lodash": "^1.1.24",
"iflow-react-router": "^1.2.1",
"jest": "^20.0.4",
"jest-cli": "^20.0.4",

This comment has been minimized.

@iansinnott

iansinnott Jul 9, 2017

Member

Maybe this wasn't necessary, but tests failed for me with a module resolution error. Not finding jest-cli.

@iansinnott

iansinnott Jul 9, 2017

Member

Maybe this wasn't necessary, but tests failed for me with a module resolution error. Not finding jest-cli.

This comment has been minimized.

@KyleAMathews

KyleAMathews Jul 11, 2017

Contributor

Huh... maybe this is necessary and tests worked because I had jest-cli installed globally? Do you not have jest-cli installed globally?

@KyleAMathews

KyleAMathews Jul 11, 2017

Contributor

Huh... maybe this is necessary and tests worked because I had jest-cli installed globally? Do you not have jest-cli installed globally?

This comment has been minimized.

@iansinnott

iansinnott Jul 11, 2017

Member

Nope, I didn't anyway. I usually use yarn test (or npm) which means there's no need for the global install. Not that i'm against it, I just liked encapsulating all deps (even globally installed ones) in package.json.

@iansinnott

iansinnott Jul 11, 2017

Member

Nope, I didn't anyway. I usually use yarn test (or npm) which means there's no need for the global install. Not that i'm against it, I just liked encapsulating all deps (even globally installed ones) in package.json.

This comment has been minimized.

@KyleAMathews

KyleAMathews Jul 12, 2017

Contributor

Ok, guess we do need this then :-)

@KyleAMathews

KyleAMathews Jul 12, 2017

Contributor

Ok, guess we do need this then :-)

Show outdated Hide outdated packages/gatsby/src/utils/webpack-modify-validate.js
console.log(
`Your Webpack config does not appear to be valid. This could be because of
something you added or a plugin. If you don't recognize the invalid keys listed
above try removing plugins and rebuilding to identify the culprit.

This comment has been minimized.

@iansinnott

iansinnott Jul 9, 2017

Member

This looks sort of ugly. Open to suggestions as to how to format multi-line strings so they look nice in code.

@iansinnott

iansinnott Jul 9, 2017

Member

This looks sort of ugly. Open to suggestions as to how to format multi-line strings so they look nice in code.

This comment has been minimized.

This comment has been minimized.

@iansinnott

iansinnott Jul 11, 2017

Member

Ah, nice. stripIndent looks like exactly what I'm looking for.

@iansinnott

iansinnott Jul 11, 2017

Member

Ah, nice. stripIndent looks like exactly what I'm looking for.

@gatsbybot

This comment has been minimized.

Show comment
Hide comment

gatsbybot commented Jul 9, 2017

Deploy preview ready!

Built with commit b23513e

https://deploy-preview-1437--gatsbygram.netlify.com

@gatsbybot

This comment has been minimized.

Show comment
Hide comment

gatsbybot commented Jul 9, 2017

Deploy preview ready!

Built with commit b23513e

https://deploy-preview-1437--gatsbyjs.netlify.com

@gatsbybot

This comment has been minimized.

Show comment
Hide comment

gatsbybot commented Jul 9, 2017

Deploy preview ready!

Built with commit b23513e

https://deploy-preview-1437--using-drupal.netlify.com

Do not validate stylus options object shape
Validating the shape felt like overkill, and it might prevent valid options form
being used if I missed something.
@iansinnott

This comment has been minimized.

Show comment
Hide comment
@iansinnott

iansinnott Jul 11, 2017

Member

Any way we can get this merged in for text release? Going to use gatsby for all the projects once Stylus support lands

Member

iansinnott commented Jul 11, 2017

Any way we can get this merged in for text release? Going to use gatsby for all the projects once Stylus support lands

@@ -30,6 +30,7 @@
"iflow-lodash": "^1.1.24",
"iflow-react-router": "^1.2.1",
"jest": "^20.0.4",
"jest-cli": "^20.0.4",

This comment has been minimized.

@KyleAMathews

KyleAMathews Jul 11, 2017

Contributor

Huh... maybe this is necessary and tests worked because I had jest-cli installed globally? Do you not have jest-cli installed globally?

@KyleAMathews

KyleAMathews Jul 11, 2017

Contributor

Huh... maybe this is necessary and tests worked because I had jest-cli installed globally? Do you not have jest-cli installed globally?

Show outdated Hide outdated packages/gatsby/src/utils/webpack-modify-validate.js
console.log(
`Your Webpack config does not appear to be valid. This could be because of
something you added or a plugin. If you don't recognize the invalid keys listed
above try removing plugins and rebuilding to identify the culprit.

This comment has been minimized.

]
```
### With CSS Modules

This comment has been minimized.

@KyleAMathews

KyleAMathews Jul 11, 2017

Contributor

For the core css setup and other style plugins e.g. sass we enable CSS Modules support automatically for file_name.module.EXT

https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-plugin-sass/src/gatsby-node.js

This has proved a nice zero-configuration way of doing things and lets a project have both raw styles and then css module styles.

@KyleAMathews

KyleAMathews Jul 11, 2017

Contributor

For the core css setup and other style plugins e.g. sass we enable CSS Modules support automatically for file_name.module.EXT

https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-plugin-sass/src/gatsby-node.js

This has proved a nice zero-configuration way of doing things and lets a project have both raw styles and then css module styles.

This comment has been minimized.

@iansinnott

iansinnott Jul 11, 2017

Member

OK, yeah I'm all for consistency. I thought about this particular issue for a bit and came to the conclusion that I've never once had a case where I wanted file-by-file control over whether or not to use CSS modules. It was always based on file type. I.e. I would not use modules with external CSS files but would use it for all my own internal stylus.

Anyway, like I said I'm all for staying consistent with the convention, but I still like the option to enable modules for the entire file type. I will see about adding both.

@iansinnott

iansinnott Jul 11, 2017

Member

OK, yeah I'm all for consistency. I thought about this particular issue for a bit and came to the conclusion that I've never once had a case where I wanted file-by-file control over whether or not to use CSS modules. It was always based on file type. I.e. I would not use modules with external CSS files but would use it for all my own internal stylus.

Anyway, like I said I'm all for staying consistent with the convention, but I still like the option to enable modules for the entire file type. I will see about adding both.

@KyleAMathews

This comment has been minimized.

Show comment
Hide comment
@KyleAMathews

KyleAMathews Jul 11, 2017

Contributor

Ugh, so sorry, I somehow never submitted my review from a couple of days ago :-(

Contributor

KyleAMathews commented Jul 11, 2017

Ugh, so sorry, I somehow never submitted my review from a couple of days ago :-(

@KyleAMathews

This comment has been minimized.

Show comment
Hide comment
@KyleAMathews

KyleAMathews Jul 11, 2017

Contributor

But yeah, definitely want to get this in :-) will make a release as soon as I can merge.

Contributor

KyleAMathews commented Jul 11, 2017

But yeah, definitely want to get this in :-) will make a release as soon as I can merge.

@iansinnott

This comment has been minimized.

Show comment
Hide comment
@iansinnott

iansinnott Jul 11, 2017

Member

Great! Will check out the review

Member

iansinnott commented Jul 11, 2017

Great! Will check out the review

@iansinnott

Just some comments, new commits to come soon

@@ -30,6 +30,7 @@
"iflow-lodash": "^1.1.24",
"iflow-react-router": "^1.2.1",
"jest": "^20.0.4",
"jest-cli": "^20.0.4",

This comment has been minimized.

@iansinnott

iansinnott Jul 11, 2017

Member

Nope, I didn't anyway. I usually use yarn test (or npm) which means there's no need for the global install. Not that i'm against it, I just liked encapsulating all deps (even globally installed ones) in package.json.

@iansinnott

iansinnott Jul 11, 2017

Member

Nope, I didn't anyway. I usually use yarn test (or npm) which means there's no need for the global install. Not that i'm against it, I just liked encapsulating all deps (even globally installed ones) in package.json.

]
```
### With CSS Modules

This comment has been minimized.

@iansinnott

iansinnott Jul 11, 2017

Member

OK, yeah I'm all for consistency. I thought about this particular issue for a bit and came to the conclusion that I've never once had a case where I wanted file-by-file control over whether or not to use CSS modules. It was always based on file type. I.e. I would not use modules with external CSS files but would use it for all my own internal stylus.

Anyway, like I said I'm all for staying consistent with the convention, but I still like the option to enable modules for the entire file type. I will see about adding both.

@iansinnott

iansinnott Jul 11, 2017

Member

OK, yeah I'm all for consistency. I thought about this particular issue for a bit and came to the conclusion that I've never once had a case where I wanted file-by-file control over whether or not to use CSS modules. It was always based on file type. I.e. I would not use modules with external CSS files but would use it for all my own internal stylus.

Anyway, like I said I'm all for staying consistent with the convention, but I still like the option to enable modules for the entire file type. I will see about adding both.

Show outdated Hide outdated packages/gatsby/src/utils/webpack-modify-validate.js
console.log(
`Your Webpack config does not appear to be valid. This could be because of
something you added or a plugin. If you don't recognize the invalid keys listed
above try removing plugins and rebuilding to identify the culprit.

This comment has been minimized.

@iansinnott

iansinnott Jul 11, 2017

Member

Ah, nice. stripIndent looks like exactly what I'm looking for.

@iansinnott

iansinnott Jul 11, 2017

Member

Ah, nice. stripIndent looks like exactly what I'm looking for.

@iansinnott

This comment has been minimized.

Show comment
Hide comment
@iansinnott

iansinnott Jul 12, 2017

Member

OK, ended up making it similar to the sass plugin: modules.styl files will get CSS modules while all other .styl files will not.

Also added the tag helper for formatting the long error message. Good rec 👍

Member

iansinnott commented Jul 12, 2017

OK, ended up making it similar to the sass plugin: modules.styl files will get CSS modules while all other .styl files will not.

Also added the tag helper for formatting the long error message. Good rec 👍

@KyleAMathews

This comment has been minimized.

Show comment
Hide comment
@KyleAMathews

KyleAMathews Jul 12, 2017

Contributor

Great! Making a minor release in a sec 💯

Contributor

KyleAMathews commented Jul 12, 2017

Great! Making a minor release in a sec 💯

@KyleAMathews

This comment has been minimized.

Show comment
Hide comment
@KyleAMathews

KyleAMathews Jul 12, 2017

Contributor

Could you make a follow-up PR with a very simple example site for Stylus? Just one (or two) components with some simple styles?

Contributor

KyleAMathews commented Jul 12, 2017

Could you make a follow-up PR with a very simple example site for Stylus? Just one (or two) components with some simple styles?

@KyleAMathews KyleAMathews merged commit 0d3783c into gatsbyjs:master Jul 12, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@KyleAMathews

This comment has been minimized.

Show comment
Hide comment
@KyleAMathews

KyleAMathews Jul 12, 2017

Contributor

Ok didn't quite get to the release. Will in an hour or so.

Contributor

KyleAMathews commented Jul 12, 2017

Ok didn't quite get to the release. Will in an hour or so.

@iansinnott

This comment has been minimized.

Show comment
Hide comment
@iansinnott

iansinnott Jul 12, 2017

Member

Yay 💯 . Thanks for being so responsive on this

Member

iansinnott commented Jul 12, 2017

Yay 💯 . Thanks for being so responsive on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment