-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
feat(haste-map): watchman crawler now includes dotfiles #10075
feat(haste-map): watchman crawler now includes dotfiles #10075
Conversation
4ac6954
to
56755b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
I thought we want to go the other way around and stop crawling dotfiles (or .git) for Node watcher: #9829 (comment) |
Hmm, interesting. But |
Ideally we'd crawl dotfiles, but make sure to ignore vcs directories. Stopping to crawl dotfiles for node was to match watchman, not the ideal solution (at least in my mind). Respecting |
I agree with crawling the dotfiles and ignoring VCS directories by default. I don't think there is a need for an option to enable crawling of VCS directories. I can update the PR if that sounds good |
SGTM :) |
56755b6
to
7af063f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
7af063f
to
f9083aa
Compare
Updated PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
packages/jest-haste-map/src/index.ts
Outdated
@@ -113,6 +113,9 @@ const CHANGE_INTERVAL = 30; | |||
const MAX_WAIT_TIME = 240000; | |||
const NODE_MODULES = path.sep + 'node_modules' + path.sep; | |||
const PACKAGE_JSON = path.sep + 'package.json'; | |||
const VCS_DIRECTORIES = ['.git', '.svn', '.hg'] | |||
.map(vcs => '/' + vcs + '/') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use ^ and $, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably use path.sep
like above as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, changed to path.sep
.
I don't think I understand why would this need ^
and $
? it matches with all the strings that include path.sep + '.git' + path.sep
and would not match with path.sep +'.github' + path.sep
.
4340280
to
5582a2d
Compare
Updated PR. also missed it last time but the watcher was ignoring the dotfiles too. Now it should emit events on dotfiles too. |
5582a2d
to
ec92955
Compare
Looks great! Thanks for doing this! 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -113,6 +114,9 @@ const CHANGE_INTERVAL = 30; | |||
const MAX_WAIT_TIME = 240000; | |||
const NODE_MODULES = path.sep + 'node_modules' + path.sep; | |||
const PACKAGE_JSON = path.sep + 'package.json'; | |||
const VCS_DIRECTORIES = ['.git', '.hg'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
svn?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meh, I have no idea how it works, and people can open a PR if needed. Should be a simple fix for anyone knowing how subversion works. And we only ship mercurial and git support anyways
@grosto seems the new tests are failing on Travis - mind taking a look? |
@SimenB looking into it |
@grosto any idea why that trailing slash only matters on Travis and not the 3 other CIs? I'd suspect one env is with watchman and the others are not or some such - is there some inconsistency? EDIT: Ah, it still failed - never mind then 😅 I still suspect some watchman/no watchman mismatch, but I might be wrong |
@SimenB I could not replicate the issue but I noticed this mismatch with other testIgnorePaths so made a blind shot. I will try to run it without watchman |
I don't trust travis, so let's try a fresh push. nodejs/docker-node#1298 (comment). While that's for the config and not code, who knows. EDIT: Didn't help 😢 No idea what's going on, I'm unable to reproduce it |
Yeah I am trying to reproduce it too, but cannot. I was thinking to rollback some changes to see if passes. It was passing(I think) before I pushed |
e1db615
to
a817726
Compare
I reverted the changes and even removed |
9c98d53
to
4253ef9
Compare
Travis says it's building this: 7449c30. Which is not HEAD of this PR... |
Meh, screw it. If it fails on master I'll delete the integration - we already build on CircleCI, GH Actions and Azure Pipelines regardless, should be covered |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Fixes #10063.
Fixes #9829.
Closes #9887.
Watchman will now crawl dotfiles. This now matches the behaviour of Node crawler.
Test plan
I added test to verify that watchman gets dotfiles and that behaviour is same as node crawler.