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.

Sign up for GitHub

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

Jump to bottom

update readme with fix from #1939 #2114

Merged
merged 10 commits into from May 12, 2017
Merged

update readme with fix from #1939 #2114

Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
80a10bf
update readme with fix from https://github.com/facebookincubator/crea…
kellyrmilligan May 10, 2017
d406724
update with better globbing
kellyrmilligan May 10, 2017
24f3fcd
added note
kellyrmilligan May 10, 2017
b3ce3f7
updating with a note and reverting previous changes
kellyrmilligan May 11, 2017
3dc41f0
how about this?
kellyrmilligan May 11, 2017
8eaaad3
refactor
kellyrmilligan May 11, 2017
1baf645
added note about new files back in after verifying locally
kellyrmilligan May 11, 2017
df67823
spaces back in, sorry was my editor!
kellyrmilligan May 11, 2017
32653a0
removed note about new files :)
kellyrmilligan May 11, 2017
2b85cfd
added bullet about new files issue currently open on node-sass
kellyrmilligan May 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 12 additions & 23 deletions packages/react-scripts/template/README.md
View file
Open in desktop
Expand Up @@ -207,7 +207,7 @@ To configure the syntax highlighting in your favorite text editor, head to the [

## Displaying Lint Output in the Editor

>Note: this feature is available with `react-scripts@0.2.0` and higher.
Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace is significant in markdown. Please leave those two spaces in, they are causing the line to break. They were intentional.

>Note: this feature is available with `react-scripts@0.2.0` and higher.
>It also only works with npm 3 or higher.

Some editors, including Sublime Text, Atom, and Visual Studio Code, provide plugins for ESLint.
Expand Down Expand Up @@ -398,15 +398,14 @@ Following this rule often makes CSS preprocessors less useful, as features like
First, let’s install the command-line interface for Sass:

```
npm install node-sass --save-dev
npm install node-sass-chokidar --save-dev
```

Then in `package.json`, add the following lines to `scripts`:

```diff
"scripts": {
+ "build-css": "node-sass src/ -o src/",
+ "watch-css": "npm run build-css && node-sass src/ -o src/ --watch --recursive",
+ "build-css": "node-sass-chokidar src/ -o src/",
+ "watch-css": "npm run build-css && node-sass-chokidar src/ -o src/ --watch --recursive",
"start": "react-scripts start",
"build": "react-scripts build",
"test": "react-scripts test --env=jsdom",
Expand All @@ -430,8 +429,8 @@ Then we can change `start` and `build` scripts to include the CSS preprocessor c

```diff
"scripts": {
"build-css": "node-sass src/ -o src/",
"watch-css": "npm run build-css && node-sass src/ -o src/ --watch --recursive",
"build-css": "node-sass-chokidar src/ -o src/",
"watch-css": "npm run build-css && node-sass-chokidar src/ -o src/ --watch --recursive",
Copy link
Contributor

@Timer Timer May 12, 2017 •
edited

Choose a reason for hiding this comment

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

Why do we build before we start watching? Does the watcher not compile everything when started in watch mode?

Copy link
Contributor

Choose a reason for hiding this comment

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

without the build-css inside of watch-css there is no guarantee the latest stylesheets are being used when you npm run start

Does the watcher not compile everything when started in watch mode?

that is correct, the watcher does not compile everything when it first starts.

  1. If it's the first time running npm run start you won't have ANY CSS files yet
  2. If you made changes to a stylesheet while you weren't watching then npm run start those changes won't show until you modify the stylesheet again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just to comment on this, I do like for example how the babel cli when started in watch mode does compile one time before the watch mode kicks in. something to think about and would simplify the scripts config.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kellyrmilligan do you want to open an issue on node-sass-chokidar requesting this change? I will make the change this weekend if you do.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you make the files compile when started in watch mode, please add a flag to disable it.
It's very annoying that babel does it because you can't say "wait for first build before starting app".

Copy link

Choose a reason for hiding this comment

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

@michaelwayman: would it be helpful to include node_modules, like so:

    "build-css": "node-sass-chokidar src/ -o src/ --include-path node_modules",
    "watch-css": "npm run build-css && node-sass-chokidar src/ -o src/ --watch --recursive --include-path node_modules",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ajs139 we actually recently added a section about that. It gives advice more about importing without relative paths similar to the NODE PATH env variable fix. I always add my src directory as well.

Copy link

Choose a reason for hiding this comment

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

Can we hide css compiled files in "npm start"?
I saw many other sources, they can run with "npm start" without compiling scss files. Imagine that my folders have many new css files, it looks ugly. Thanks for any help!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the initial terminal output? If so yes you can pass -q to build-css like so:
npm run build-css -- -q, this will call it in quiet mode. if this is not why you mean, create react app does not let you ATM add in custom loaders for scss, so this is a workaround to allow for that. In practice it has t been a big deal tbh

Copy link

@kidqn kidqn Jun 9, 2017 •
edited

Choose a reason for hiding this comment

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

Thank to your reply. I mean that I want to use "node-sass-chokidar" but still can import .scss files.
For example: my Home folder includes: Home.js, Home.scss, ...
After run "npm start", my folder add Home.css. So I'm scared of that one day I forget and edit css files instead of scss.
Another issue is that it's assumed that I have Styles folder including 10 SCSS files, and now after run "npm start" it will have 20 files. I knew we can ignore CSS files, but it makes structure of app folders cumbrous.

In some react apps built with webpack 2 as I did, we need just import .scss files, can run "npm start" in dev mode without compiling new css files. Therefore, can you improve it? example, build an entire css and hide it somewhere

Copy link
Contributor

Choose a reason for hiding this comment

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

like @kellyrmilligan said, custom scss loaders are not supported at the moment.

  • One option you have available is to npm run eject and add a scss loader to config/webpack.config.*.js. then you should be able to import 'styles.scss';
  • Another option is to output the css to a separate folder with node-sass-chokidar with the -o <path> option, however, this will make importing the css a huge pain if you use self-contained component hierarchy pattern like I do.
  • also, if you add *.css to .gitignore, and you configure your IDE or VIM or Emacs or w.e. to not show css files then maybe it won't seem so cumbrous?

anyways, perhaps you can convince the visionaries working on this to let you open a PR providing an entry point or env variable into the web-pack-config WITHOUT running eject?

Copy link

@kidqn kidqn Jun 9, 2017 •
edited

Choose a reason for hiding this comment

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

@michaelwayman : Thanks. That's exactly I want.
I tried the second way, edit web-pack-config, run eject to add scss loader and eventually it failed.
The node-sass-chokidar's way works fine, so I think something likes scss loader will make create-react-app better :) I still dont know the reason why create-react-app didn't suppor SCSS.

P/S: I'm using VS, and config it to hide css files but it should be not the best method (import from a invisible file)

- "start": "react-scripts start",
- "build": "react-scripts build",
+ "start-js": "react-scripts start",
Expand All @@ -442,27 +441,17 @@ Then we can change `start` and `build` scripts to include the CSS preprocessor c
}
```

Now running `npm start` and `npm run build` also builds Sass files. Note that `node-sass` seems to have an [issue recognizing newly created files on some systems](https://github.com/sass/node-sass/issues/1891) so you might need to restart the watcher when you create a file until it’s resolved.
Now running `npm start` and `npm run build` also builds Sass files. Note that `node-sass-chokidar` seems to have an issue recognizing newly created files on some systems so you might need to restart the watcher when you create a file until it’s resolved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that node-sass-chokidar seems to have an issue recognizing newly created files

I would like to comment though that node-sass-chokidar does not have a problem recognizing new files.

@Timer thanks for the tag, this is amazing :)

If you want to see why node-sass has this problem, read on:

You can see here that node-sass only watches files that are there when program first runs. And possibly newly created files IFF they are imported by a file that IS being watched.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, doesn't this contradict:

added note about new files back in after testing

@kellyrmilligan Can you confirm you still experience the issue with new files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michaelwayman in testing it seems to not recognize a new file being added for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is a good way to verify it? I am testing on os x sierra.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, thanks @kellyrmilligan
can you open an issue on node-sass-chokidar and describe steps to reproduce, as well as your system details. (e.g. OS, node, npm, and package versions) thanks so much and I will try to have this fixed ASAP

It works as expected for me so I need to try and duplicate your setup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, what I did to test it was:

  1. Start a clean project and install node-sass-chokidar.
  2. run node-sass-chokidar src/ -o src/ --watch --recursive
  3. Create a new scss file src/my_file.scss with simple style #yoyo {color:red;} and save.
    • note that the file HAS to be in the directory that's being watched i.e. src/
  4. Check for the existence of src/my_file.css

that was all the "testing" I did but it works fine. So yeah, please describe your steps to reproduce and system info in an issue here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized I tried a _test.scss file, which is meant to be included in others. in testing a regular file you are correct and i've removed the note about that.


**Performance Note**
**Why `node-sass-chokidar`?**

`node-sass --watch` has been reported to have *performance issues* in certain conditions when used in a virtual machine or with docker. If you are experiencing high CPU usage with node-sass you can alternatively try [node-sass-chokidar](https://www.npmjs.com/package/node-sass-chokidar) which uses a different file-watcher. Usage remains the same, simply replace `node-sass` with `node-sass-chokidar`:
`node-sass` has been reported as having the following issues:

```
npm uninstall node-sass --save-dev
npm install node-sass-chokidar --save-dev
```
- `node-sass --watch` has been reported to have *performance issues* in certain conditions when used in a virtual machine or with docker.

And in your scripts:
- Infinite styles compiling [#1939](https://github.com/facebookincubator/create-react-app/issues/1939)

```diff
"scripts": {
- "build-css": "node-sass src/ -o src/",
- "watch-css": "npm run build-css && node-sass src/ -o src/ --watch --recursive"
+ "build-css": "node-sass-chokidar src/ -o src/",
+ "watch-css": "npm run build-css && node-sass-chokidar src/ -o src/ --watch --recursive"
}
```
`node-sass-chokidar` is used here as it addresses these issues.

## Adding Images, Fonts, and Files

Expand Down