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

Refactor extra watch options regex to react-dev-utils #3362

Merged
merged 8 commits into from Nov 4, 2017

Conversation

@xjlim
Copy link
Contributor

commented Oct 30, 2017

Refactor based on @gaearon comment

@Timer
Copy link
Collaborator

left a comment

This does not seem functionally equivalent.

.replace(/[\\]+/g, '\\\\')}).+[\\\\/]node_modules[\\\\/]`,
'g'
),
ignored: watchOptionsRegex(paths),

This comment has been minimized.

Copy link
@Timer

Timer Oct 31, 2017

Collaborator

We can probably pass paths.appSrc instead of paths.

const path = require('path');
const escapeStringRegexp = require('escape-string-regexp');

module.exports = function watchOptionsRegex(paths) {

This comment has been minimized.

Copy link
@Timer

Timer Oct 31, 2017

Collaborator

I'm not fond of this name, I'd like to point out these are the ignored files.


module.exports = function watchOptionsRegex(paths) {
return new RegExp(
`^(?!${escapeStringRegexp(path.join(paths.appSrc, '/'))}).+node_modules`,

This comment has been minimized.

Copy link
@Timer

Timer Oct 31, 2017

Collaborator

This regex does not look identical to the old regex -- why the changes? They won't be functionally equivalent afaik.

This comment has been minimized.

Copy link
@Timer

Timer Oct 31, 2017

Collaborator

Since this its own package now, why don't we add a few test cases to check paths that this regex should be true/false against?

This comment has been minimized.

Copy link
@xjlim

xjlim Oct 31, 2017

Author Contributor

@Timer I added the test cases, how should they be wired up in the CI scripts?

This comment has been minimized.

Copy link
@Timer

Timer Oct 31, 2017

Collaborator

Probably just call the tests in e2e simple similar to error overlay.

@Timer

This comment has been minimized.

Copy link
Collaborator

commented Oct 31, 2017

Thanks for taking this on!

@Timer Timer added this to the 1.0.x milestone Oct 31, 2017

@Timer Timer added the tag: internal label Oct 31, 2017

@gaearon gaearon force-pushed the facebook:master branch from b544611 to a9257ec Oct 31, 2017

@gaearon

This comment has been minimized.

Copy link
Member

commented Oct 31, 2017

Sorry, I messed up git history. Would you mind doing

git fetch origin

Then write down hashes of your commits, do git reset --hard origin/master and cherry-pick them back onto your branch.

@xjlim xjlim force-pushed the xjlim:extract-regex-to-dev-utils branch from c59e208 to d252e9a Oct 31, 2017

@xjlim

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2017

@Timer I changed the regex as it was not working on windows. After digging into it, it turns out that the previous lookahead regex fails with windows path as anymatch test against both the path and the normalized version

@Timer

This comment has been minimized.

Copy link
Collaborator

commented Nov 2, 2017

Seems to be broken still. I believe the reason for the wild regex (which you simplified) was cross OS support. Can you try the old one exactly?

@xjlim

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2017

What is broken with the current regex? The old one did not work when I tested it on my Windows machine. It was cross OS but the regex fails in anymatch:

appSrc = 'C:\\foo';
regex = /^(?!C:\\foo\\).+[\\/]node_modules[\\/]/
file = 'C:\\foo\\node_modules\\Test.js'

result = criterion.test(string) || altString && criterion.test(altString);
expect to be false but is true as anymatch regex is false for the first case but is true for the normalize path - C:/foo/node_modules/Test.js as this will not match the negative lookahead

xjlim and others added 2 commits Nov 2, 2017
@Timer
Timer approved these changes Nov 4, 2017
@Timer

This comment has been minimized.

Copy link
Collaborator

commented Nov 4, 2017

Thanks!

@Timer Timer merged commit 36cd35d into facebook:master Nov 4, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
placenamehere added a commit to automatastudios/create-react-app that referenced this pull request Nov 30, 2017
Refactor extra watch options regex to react-dev-utils (facebook#3362)
* extra watch options regex to react-dev-utils

* fix regex

* add test

* fix eslint error

* include react-dev-utils test in CI script

* attempt to fix import error

* attempt to fix error on CI

* Update .eslintrc
@gaearon gaearon referenced this pull request Jan 15, 2018
tabrindle added a commit to tabrindle/create-react-app that referenced this pull request Jan 18, 2018
Refactor extra watch options regex to react-dev-utils (facebook#3362)
* extra watch options regex to react-dev-utils

* fix regex

* add test

* fix eslint error

* include react-dev-utils test in CI script

* attempt to fix import error

* attempt to fix error on CI

* Update .eslintrc
Pavek pushed a commit to Pavek/create-react-app that referenced this pull request Jul 10, 2018
Refactor extra watch options regex to react-dev-utils (facebook#3362)
* extra watch options regex to react-dev-utils

* fix regex

* add test

* fix eslint error

* include react-dev-utils test in CI script

* attempt to fix import error

* attempt to fix error on CI

* Update .eslintrc
zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Aug 14, 2018
Refactor extra watch options regex to react-dev-utils (facebook#3362)
* extra watch options regex to react-dev-utils

* fix regex

* add test

* fix eslint error

* include react-dev-utils test in CI script

* attempt to fix import error

* attempt to fix error on CI

* Update .eslintrc

@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
4 participants
You can’t perform that action at this time.