Skip to content
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

Sass loader #4195

Merged
merged 28 commits into from
Apr 18, 2018
Merged

Sass loader #4195

merged 28 commits into from
Apr 18, 2018

Conversation

Fabianopb
Copy link
Contributor

@Fabianopb Fabianopb commented Mar 21, 2018

Opt-in support for SASS and CSS-modules using SASS files added, using both .scss and .sass extensions.
This PR:

  • Installs sass-loader in the react-scripts package
  • Adds webpack configuration to allow importing scss/sass directly into components during development, supporting also the use of CSS-modules
  • Adds webpack configuration for bundling scss/sass files into the optimized production build, supporting also the use of CSS-modules
  • CSS-modules in SASS are supported in a similar way than using purely CSS, which in this case is naming SASS files as .module.scss or .module.sass.

~No automated test just yet (coming in a couple days)! However I've manually tried the e2e flow using both scss and sass extensions, with or without modules, and it worked just fine.~~
edit: ✅ Unit and integration tests added!

This PR depends on webpack-contrib/sass-loader#533 to work properly. ✅ Merged and integrated!

I'll be happy to get comments! :)

edit: @andriijas mentioned that we should use react-dev-utils/getCSSModuleLocalIdent when #4192 is merged, for the localIdentName. ✅ Merged and integrated!

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@andriijas
Copy link
Contributor

Great work!

We are adding a lot of config which is very similar to .css and .module.css

can we make a function that removes some duplication? If we make such a function it should be a breeze to support both less, less modules, scss, scss modules, css and css modules. they all have more or less equivalent config.

minimize: true,
sourceMap: shouldUseSourceMap,
modules: true,
localIdentName: '[path]__[name]___[local]',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use react-dev-utils/getCSSModuleLocalIdent when #4192 is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@gaearon
Copy link
Contributor

gaearon commented Mar 24, 2018

Can somebody also take over webpack-contrib/sass-loader#533 since @Timer is busy with other things?

@Fabianopb
Copy link
Contributor Author

@andriijas, that's a good point, I've abstracted the common options in the style loaders to a function, PR updated.

@Timer
Copy link
Contributor

Timer commented Mar 25, 2018

I have updated webpack-contrib/sass-loader#533 and it should now be merge-ready.

@Fabianopb
Copy link
Contributor Author

@andriijas, I got the unit and integration tests working a couple days ago, I guess now it's just waiting for webpack-contrib/sass-loader#533 and #4192

@iansu iansu merged commit bf3d73c into facebook:next Apr 18, 2018
@iansu
Copy link
Contributor

iansu commented Apr 18, 2018

Thanks @Fabianopb and everyone else who helped out!

@Fabianopb
Copy link
Contributor Author

Thank you all, what a journey! 😄

@gaearon
Copy link
Contributor

gaearon commented Apr 18, 2018

Wow

@Tasemu
Copy link

Tasemu commented Apr 19, 2018

Excellento!

@miraage
Copy link

miraage commented Apr 21, 2018

@Fabianopb @Timer @gaearon what do you think about providing an option to configure sass? We love using includePaths for instance..

@Timer
Copy link
Contributor

Timer commented Apr 21, 2018

@miraage I don't believe so, unless you can show some really compelling reasons and we can come up with a sensible default. This would fall under the same reasoning we don't allow you to set webpack aliases/etc.

@miraage
Copy link

miraage commented Apr 21, 2018

@Timer, I find it useful to keep variables/mixins somewhere in src/styles/mixins.scss and I put src/styles to scss includePaths so I can import it like @import "mixins"; at any deep level.

kellyrmilligan added a commit to kellyrmilligan/create-react-app that referenced this pull request May 2, 2018
* upstream/next: (35 commits)
  Update envinfo and issue template (facebook#4375)
  Update sass-loader to 7.0.1 (facebook#4376)
  Support package distribution tags (facebook#4350)
  fix broken css module support in prod (facebook#4361)
  Bumped jest version to 22.4.1 (facebook#4362)
  bump babel 7 to beta 46
  bump lint-staged to node 10 compatible version
  documentation: Added License to the README.md (facebook#4294)
  Bump `fsevents`. (facebook#4331)
  Fix typo in e2e-simple.sh comment (facebook#4323)
  Add Sass loader (facebook#4195)
  Fix some typos in README.md (facebook#4286)
  Added learnstorybook.com to Storybook links (facebook#4298)
  Document multiple build environments via `env-cmd` facebook#4071 (facebook#4117)
  Fixed link to CSS imports blog post
  Update CSS Modules localIndetName (facebook#4192)
  Enable loose mode for `class-properties` (facebook#4248)
  bump babel 7 beta (facebook#4253)
  Small typo fix facebook#4217
  Changelog for 1.1.4
  ...
kellyrmilligan added a commit to kellyrmilligan/create-react-app that referenced this pull request May 2, 2018
* next: (35 commits)
  Update envinfo and issue template (facebook#4375)
  Update sass-loader to 7.0.1 (facebook#4376)
  Support package distribution tags (facebook#4350)
  fix broken css module support in prod (facebook#4361)
  Bumped jest version to 22.4.1 (facebook#4362)
  bump babel 7 to beta 46
  bump lint-staged to node 10 compatible version
  documentation: Added License to the README.md (facebook#4294)
  Bump `fsevents`. (facebook#4331)
  Fix typo in e2e-simple.sh comment (facebook#4323)
  Add Sass loader (facebook#4195)
  Fix some typos in README.md (facebook#4286)
  Added learnstorybook.com to Storybook links (facebook#4298)
  Document multiple build environments via `env-cmd` facebook#4071 (facebook#4117)
  Fixed link to CSS imports blog post
  Update CSS Modules localIndetName (facebook#4192)
  Enable loose mode for `class-properties` (facebook#4248)
  bump babel 7 beta (facebook#4253)
  Small typo fix facebook#4217
  Changelog for 1.1.4
  ...
@lukaszgolder
Copy link

@Timer Maybe instead of providing possibility to configure sass you can include NODE_PATH in the includePaths same way it is done with modules.

It would be something like this:
includePaths: process.env.NODE_PATH.split(path.delimiter).filter(Boolean)

@Timer
Copy link
Contributor

Timer commented May 12, 2018

Can you please open an issue for this @lukaszgolder? Thanks!

@devxpy
Copy link

devxpy commented May 22, 2018

When shall this be merged with master?

@bugzpodder
Copy link

Hey devxpy, this will be released as part of CRA 2.0 and we don't yet have a definitive timeline. To try out the alpha build, please follow instructions on #3815

@terebentina
Copy link

Just upgraded cra 1 to cra2 on an existing project and sass support seems to require node-sass to be installed in the project's deps. Is this something that didn't make it into 2.0.0-next.66cc7a90? The commits above seem to suggest it shouldn't be the case.

Also, on a different topic: the docs for 1.x suggest to use node-sass-chokidar. Are we ok to use node-sass now?

@samuelcolvin
Copy link

see #2498 (comment) I think, the agreement was that users need to manually install node-sass.

@Fabianopb
Copy link
Contributor Author

Yes @terebentina, node-sass is required, you can also refer to webpack-contrib/sass-loader#533 for more information.

@jimbojetlag
Copy link

@miraage I don't believe so, unless you can show some really compelling reasons and we can come up with a sensible default. This would fall under the same reasoning we don't allow you to set webpack aliases/etc.

@Timer please see #4651 where it is explains that npm packages importing other npm packages depend on the node_modules/ sass path. Maybe I'm missing something here, but this use case is an obvious one, and it alone should be enough as a compelling reason.

It does not make sense that you require users of CRA to manually install node-sass, yet you do not allow any options for custom sass paths, while not providing sane defaults. I do not see how the users can shoot themselves in the foot if you expose that option.

As many users have already suggested, there is nothing wrong with using includePaths, this is the way sass was designed, and has been working for many years. Your ultimate dependency here is sass, not sass-loader. Sass exposes includePaths and that's what should be used.

Relying on sass-loader for this simple config may not be a good idea, the devs of sass-loader have been keep moving back and forth between webpack and sass for this config, causing regressions.

It is sad to see that the javascript community keeps breaking what works perfectly.

@devxpy
Copy link

devxpy commented Jun 20, 2018

@jimbojetlag I have a project, react-pages which let's you do this using the NODE_PATH env variable

(Enabled by default using .env ofc.)

@jarretmoses
Copy link

I am probably missing something but could someone explain how CRA will be initialized with SASS support? (ie what’s the cli command)

@jarretmoses
Copy link

@matgargano thanks for this but I meant how to use ‘sass-loader’ rather than the approach detailed in the docs. (node-sass-chokidar). I know I can just eject but this PR seems to say this will be a built in feature.

@matgargano
Copy link

@jarretmoses what are you trying to accomplish that that URL does not provide a way to do?

@Timer
Copy link
Contributor

Timer commented Jul 2, 2018

@jarretmoses if you install the beta (npm upgrade react-scripts@next --exact), just import a sass file. It will provide you with any instructions necessary and then proceed to "just work".

Please open an issue if you need any more assistance.

@facebook facebook locked as resolved and limited conversation to collaborators Jul 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.