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

Chokidar Updates #4205

Merged
merged 4 commits into from
May 20, 2018
Merged

Chokidar Updates #4205

merged 4 commits into from
May 20, 2018

Conversation

originell
Copy link
Contributor

@originell originell commented Mar 23, 2018

There have been a couple of changes to node-sass-chokidar over the recent days.

The following things have been changed, according to the node-sass-chokidar/CHANGELOG:

  • --recursive flag removed as it doesn't do anything (CHANGELOG v1.0.0)
  • initial scss build removed on watch-css, as chokidar changed the behaviour (CHANGELOG v1.0.0)

🍻

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

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

@Timer
Copy link
Contributor

Timer commented Mar 26, 2018

/cc @michaelwayman I would love your LGTM

Copy link
Contributor

@petetnt petetnt left a comment

Choose a reason for hiding this comment

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

Just a tiny nit 👍

@@ -584,7 +584,7 @@ To enable importing files without using relative paths, you can add the `--incl

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

@petetnt petetnt Mar 26, 2018

Choose a reason for hiding this comment

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

Remove another --recursive flag here

Copy link
Contributor

Choose a reason for hiding this comment

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

the --recursive flag was removed in 1.0.0 and recursion is always on because there is no way to be non-recursive with chokidar.

Other than that this LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

also the --include-path ./src isn't necessary, let's say you have files ./src/index.scss and ./src/_mixins.scss and in index.scss you @import './_mixins' it will still import correctly.

You don't necessarily need the --include-path ./src

Thanks @originell for keeping the docs updated with node-sass-chokidar, it had slipped my mind.

@originell
Copy link
Contributor Author

Changes landed. Also found another build-css I missed earlier. Hope that it's all good now :)

@michaelwayman
Copy link
Contributor

michaelwayman commented Mar 27, 2018

@Timer LGTM

Copy link
Contributor

@petetnt petetnt left a comment

Choose a reason for hiding this comment

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

LGTM

@iansu
Copy link
Contributor

iansu commented Apr 10, 2018

Do we want this going into master or next?

@michaelwayman
Copy link
Contributor

This should go ahead and get merged now. node-sass-chokidar. Thanks @originell for the PR, this LGTM.

@gaearon
Copy link
Contributor

gaearon commented May 20, 2018

@iansu We don't plan to make more changes to master aside from critical fixes.

@gaearon gaearon changed the base branch from master to next May 20, 2018 22:27
According to the changelog it is not necessary anymore as it doesn't do anything.
Chokidar now does this on it's own.
@gaearon gaearon merged commit 0dfc6f6 into facebook:next May 20, 2018
@Timer Timer added this to the 2.0.0 milestone May 21, 2018
zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
* Removes Chokidar Recursive Flag

According to the changelog it is not necessary anymore as it doesn't do anything.

* Removes initial build on SCSS watch

Chokidar now does this on it's own.

* Removes sass watch recursive, default include-path

as proposed by @michaelwayman

* Removes another left-over build-css
@lock lock bot locked and limited conversation to collaborators Jan 19, 2019
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.

7 participants