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

Allow the dev server to watch for changes in src/node_modules #2760

Closed
wants to merge 1 commit into from

Conversation

bmac
Copy link

@bmac bmac commented Jul 10, 2017

This allows users to put a node_modules folder in src if they want to use absolute imports
or imitate the webpack resolve.alias config.

This allows users to put a node_modules folder in src if they want to use absolute imports
or imitate the webpack resolve.alias config.

When experimenting with the src/node_modules trick mentioned in #1065 I noticed that the dev server wasn't picking up any changes that happened in the src/node_modules folder. This change makes it so the dev server will correctly notice changes in src/node_modules and rebuild while still ignoring the node_modules folder in the root of the project.

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

Copy link
Contributor

@Timer Timer left a comment

Choose a reason for hiding this comment

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

Thanks so much for this catch!
Can you please make sure this works on Windows?

watchOptions: {
ignored: /node_modules/,
ignored: /^([^\/]|\/(?!src))*\/node_modules\//,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work on Windows? Strictly checking for / might be problematic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I believe this "breaks" for things like src2, as it checks for a prefix of src. Major edgecase.

Copy link
Author

Choose a reason for hiding this comment

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

I'll update this for Windows.

I'm not familiar with the src2 edgecase. Do you mind elaborating more on this?

@Timer Timer modified the milestones: 1.x, 1.0.x Jul 14, 2017
This allows users to put a `node_modules` folder in `src` if they want to use absolute imports
or imitate the webpack `resolve.alias` config.
@bmac
Copy link
Author

bmac commented Jul 18, 2017

I've updated this pr to work for windows. I'm not sure I understand the src2 edgecase well enough to fix that issue at this time but any feedback would be helpful.

@Timer
Copy link
Contributor

Timer commented Sep 24, 2017

Sorry for letting this slip through the cracks.

Effectively, any directory which starts with src will have its node_modules watched.
This may not be desirable.

Also, I'm not entirely sure what file path is being examined; what if a parent folder is named src (assuming full file paths)?
If these are relative paths we may be fine.

I'm thinking we might need to generate the regex dynamically with an explicit allowance for /projectpath/src/**/node_modules.

@react-scripts-dangerous

Hello! I'm a bot that helps facilitate testing pull requests.

Your pull request (commit 1e71d47) has been released on npm for testing purposes.

npm i react-scripts-dangerous@1.0.11-1e71d47.0
# or
yarn add react-scripts-dangerous@1.0.11-1e71d47.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.11-1e71d47.0 folder/

Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk!

Thanks for your contribution!

@xjlim
Copy link
Contributor

xjlim commented Oct 2, 2017

Hi @bmac,
I have created a new PR based off this to try to address @Timer's concerns.

@Timer Timer closed this in 1e98d0f Oct 3, 2017
zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Aug 14, 2018
* Allow the dev server to watch for changes in src/node_modules

* fix eslint error

* fix broken regex

* handle trailing slash edge case for file paths

Closes facebook#2760
Fixes facebook#3223
@lock lock bot locked and limited conversation to collaborators Jan 20, 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.

None yet

5 participants