react-scripts@next drops css import in production build

[cdaringe]

Is this a bug report?

yes

Did you try recovering your dependencies?

yes.

$ yarn --version
1.9.0-20180706.1003

Which terms did you search for in User Guide?

n/a

Environment

Environment:
  OS:  macOS High Sierra 10.13.6
  Node:  10.10.0
  Yarn:  1.9.0-20180706.1003
  npm:  6.4.1
  Watchman:  Not Found
  Xcode:  Xcode 9.4.1 Build version 9F2000
  Android Studio:  Not Found

Packages: (wanted => installed)
  react: ^16.5.2 => 16.5.2
  react-dom: ^16.5.2 => 16.5.2
  react-scripts: next => 2.0.0

Steps to Reproduce

(Write your steps here:)

1. clone https://github.com/cdaringe/will-u-load-my-styl/
2. observe that there are exactly two commits. one is a fresh commit of npx create-react-app@next, the other adds a component lib & style sheet
3. yarn start. observe a styles applied. kill the process
4. yarn build. serve the production build. observe missing styles.

originally discovered here

Expected Behavior

• imported stylesheet should be included in bundle

Actual Behavior

• imported stylesheet is missing

more specfically, here, a css file is imported as shown below:

import 'react-octagon/lib/styles/semantic.css'
...

this dependency shows up in dev mode in the dev server, but no such CSS is generated into the build dir.

interestingly, importing the desired css file from within another CSS file produces expected results. see @youngbob's remark here.

Reproducible Demo

• https://github.com/cdaringe/will-u-load-my-styl

instructions listed above.

[edmorley]

Hi @cdaringe :-)

The react-octagon package has set sideEffects: false in their package.json:
https://github.com/Tripwire/octagon/blob/v15.1.2/package.json#L98

As such when mode is 'production' webpack 4's improved tree shaking drops the import by design.

From the webpack tree shaking docs:

Note that any imported file is subject to tree shaking. This means if you use something like css-loader in your project and import a CSS file, it needs to be added to the side effect list so it will not be unintentionally dropped in production mode:

Therefore react-octagon's package.json needs to instead use something like:

  "sideEffects": [
    "*.css"
  ],

[01dr]

This is it. I tried this case with a other library, and everything works correctly

[cdaringe]

cool, thanks all. it was because of side effects indeed, but just to clarify, i don't think the prescribed solution is the ideal solution. the root cause is really that i simply didn't use that import.

// works.
import * as css from 'react-octagon/lib/styles/semantic.css'
console.log(css) // can't shake me now, i'm used!

thus, because my code didn't use the css, it was shaken. i would think in my CRA project (not my dependencies) i would declare sideEffects: [ <css glob > ] to make webpack hold onto any css file. this does not work for the aforementioned example, but IMHO matches the language used in the webpack docs.

having the whole thing work by declaring css sideEffects inreact-octagon seems off. from react-octagon's perspective, a css file doesn't fit the definition of a sideEffect--it's a logicless asset.

what perspective am i missing here? maybe this conversation is better suited for the webpack repo :)

[edmorley]

@cdaringe it's valid to have imports that are not assigned to anything.

The issue here is that webpack 4 created a new way for packages to declare "none/some/all of the files in this package have any side-effects by importing alone, so it's safe to drop them as an optimization", via the optional sideEffects entry in package.json. And that API currently requires packages to list all files including CSS, since webpack doesn't special-case particular types (and doesn't offer a way for its loaders to do so either).

As such at least in the feature's current implementation, omitting CSS file entries from the sideEffects property is a bug.

However I agree the sideEffects feature should be improved - discussion in webpack/webpack#6571.