Sass loader by Fabianopb · Pull Request #4195 · react/create-react-app

Fabianopb · 2018-03-21T20:38:03Z

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/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!

…uction build.

facebook-github-bot · 2018-03-21T20:39:17Z

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 cla@fb.com. Thanks!

facebook-github-bot · 2018-03-21T20:49:22Z

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

andriijas · 2018-03-22T09:59:02Z

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.

andriijas · 2018-03-22T10:12:09Z

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


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

…ronment.

gaearon · 2018-03-24T13:50:49Z

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

Fabianopb · 2018-03-24T14:14:04Z

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

Timer · 2018-03-25T22:11:22Z

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

…hout modules.

Fabianopb · 2018-03-29T05:30:35Z

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

…nto sass-loader

iansu · 2018-04-18T16:36:41Z

Thanks @Fabianopb and everyone else who helped out!

Fabianopb · 2018-04-18T16:46:03Z

Thank you all, what a journey! 😄

gaearon · 2018-04-18T16:48:23Z

Wow

Tasemu · 2018-04-19T08:49:15Z

Excellento!

miraage · 2018-04-21T20:46:12Z

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

Timer · 2018-04-21T20:49:36Z

@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 · 2018-04-21T21:21:54Z

@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.

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

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

lukaszgolder · 2018-05-12T10:48:54Z

@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 · 2018-05-12T19:09:15Z

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

devxpy · 2018-05-22T17:39:03Z

When shall this be merged with master?

bugzpodder · 2018-05-22T18:08:33Z

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 · 2018-05-24T16:42:04Z

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 · 2018-05-24T16:46:20Z

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

Fabianopb · 2018-05-24T18:00:34Z

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

jimbojetlag · 2018-06-20T13:00:43Z

@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 · 2018-06-20T18:09:20Z

@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 · 2018-07-02T13:26:58Z

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

matgargano · 2018-07-02T15:22:56Z

@jarretmoses check out https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/template/README.md#adding-a-css-preprocessor-sass-less-etc

jarretmoses · 2018-07-02T15:29:40Z

@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 · 2018-07-02T15:50:17Z

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

Timer · 2018-07-02T16:53:49Z

@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.

Fabianopb added 8 commits March 20, 2018 19:32

Installs and adds sass loader task in webpack for dev environment.
4b40a7d

Uses Timer's branch of sass-loader without node-sass dependency.
0670888

Adds method for handling SASS modules.
fc1b9d4

Fixes extension of excluded files when looking for scss modules.
ac4cf34

Adds support for both .scss and .sass extensions.
61bd312

Uses ExtractTextPlugin with sass-loader to bundle styles for the prod…
69ce2e6
…uction build.

Bundles SASS modules for the production build.
0a81f2a

Uses the latest version of sass-loader.
5a7f534

facebook-github-bot added the CLA Signed label Mar 21, 2018

andriijas previously requested changes Mar 22, 2018
View reviewed changes

iansu mentioned this pull request Mar 23, 2018
Helpful if modules react-svg-loader and sass-loader get added. #4199
Closed

Fabianopb added 3 commits March 24, 2018 15:12

Adds function to create different rules for style loaders in dev envi…
b2573f5
…ronment.

Abstracts style loaders to a common function to avoid repetition.
6e60b6e

Simplifies the common function that creates style loaders.
c8052c5

samuelcolvin mentioned this pull request Mar 26, 2018
Optional Sass support - Part 2 #2498
Closed

Fabianopb added 7 commits March 26, 2018 21:34

Creates assets for testing SASS/SCSS support.
876c9b3

Creates mock components and unit tests for SASS and SCSS with and wit…
9865439
…hout modules.

Creates integration tests for SASS/SCSS support.
1629d78

Adds node-sass as a template dependency so sass-loader can be tested.
479423a

Includes sass tests when test component is mounted.
dc640e4

Fixes asserted module name for sass and scss modules tests.
f44250e

Removes tests against css imports in SCSS and SASS files.
0f36d90

Merge branch 'next' ofhttps://github.com/facebook/create-react-appi…
c68d168
…nto sass-loader

Use importLoaders config according to css-loader docs.
22475dc

iansu approved these changes Apr 18, 2018
View reviewed changes

iansu merged commit bf3d73c into react:nextApr 18, 2018

gaearon mentioned this pull request Apr 21, 2018
Roadmap for react-scripts@2.0 #3815
Closed

gudh mentioned this pull request May 20, 2018
create-react-app 2.0, how to use absolute path import in sass/scss files? #4494
Closed

wongbsn mentioned this pull request Jun 1, 2018
Unify to have 1 way of handling scss Experience-Monks/nyg-jam3#119
Closed

react locked as resolved and limited conversation to collaborators Jul 2, 2018

Uh oh!

Uh oh!

Conversation

Fabianopb commented Mar 21, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Mar 21, 2018

Uh oh!

facebook-github-bot commented Mar 21, 2018

Uh oh!

andriijas commented Mar 22, 2018

Uh oh!

andriijasMar 22, 2018

Choose a reason for hiding this comment

Uh oh!

FabianopbApr 14, 2018

Choose a reason for hiding this comment

Uh oh!

gaearon commented Mar 24, 2018

Uh oh!

Fabianopb commented Mar 24, 2018

Uh oh!

Timer commented Mar 25, 2018

Uh oh!

Fabianopb commented Mar 29, 2018

Uh oh!

iansu commented Apr 18, 2018

Uh oh!

Fabianopb commented Apr 18, 2018

Uh oh!

gaearon commented Apr 18, 2018

Uh oh!

Tasemu commented Apr 19, 2018

Uh oh!

miraage commented Apr 21, 2018

Uh oh!

Timer commented Apr 21, 2018

Uh oh!

miraage commented Apr 21, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukaszgolder commented May 12, 2018

Uh oh!

Timer commented May 12, 2018

Uh oh!

devxpy commented May 22, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bugzpodder commented May 22, 2018

Uh oh!

terebentina commented May 24, 2018

Uh oh!

samuelcolvin commented May 24, 2018

Uh oh!

Fabianopb commented May 24, 2018

Uh oh!

jimbojetlag commented Jun 20, 2018

Uh oh!

devxpy commented Jun 20, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarretmoses commented Jul 2, 2018

Uh oh!

matgargano commented Jul 2, 2018

Uh oh!

jarretmoses commented Jul 2, 2018

Uh oh!

matgargano commented Jul 2, 2018

Uh oh!

Timer commented Jul 2, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

17 participants

Fabianopb commented Mar 21, 2018•
edited
Loading

miraage commented Apr 21, 2018•
edited
Loading

devxpy commented May 22, 2018•
edited
Loading

devxpy commented Jun 20, 2018•
edited
Loading

Timer commented Jul 2, 2018•
edited
Loading