jest-haste-map: watchman crawler: normalize paths #3887
Conversation
|
Can you rebase this? |
| @@ -104,7 +104,7 @@ module.exports = function watchmanCrawl( | |||
|
|
|||
| clocks[root] = response.clock; | |||
| response.files.forEach(fileData => { | |||
| const name = root + path.sep + fileData.name; | |||
| const name = path.normalize(path.join(root, fileData.name)); | |||
There was a problem hiding this comment.
The initial implementation used to have this, however it is a source of significant slowdown (~20% of the entire time spent in jest-haste-map). Is there something else we could do here?
There was a problem hiding this comment.
Oh, got it. Is it also for performance that path.join wasn't used?
The alternative would be to use a RegExp to replace all / with path.sep. Do you think that'd be reasonnable? It sounds like the performance would be better because it wouldn't have to do any parsing that normalize is probably doing.
There was a problem hiding this comment.
Do you mind doing some benchmarking on what is fastest on node8? Things may have changed and the path module was rewritten for node 6 also.
There was a problem hiding this comment.
Well, you known what, the thing is we don't really need to 'normalize' paths logically speaking. Watchman ensures that the paths don't contain any ".." for example, so that's unnecessary work to parse and reform the paths. It's really just a matter of slashes/backslashes. I'll just do a regex replacement.
jeanlauliac
force-pushed
the
normalize-paths
branch
from
a6c3d6b
to
27c749b
Compare
Codecov Report
@@ Coverage Diff @@
## master #3887 +/- ##
==========================================
+ Coverage 58.09% 58.12% +0.03%
==========================================
Files 195 196 +1
Lines 6751 6756 +5
Branches 6 6
==========================================
+ Hits 3922 3927 +5
Misses 2826 2826
Partials 3 3
Continue to review full report at Codecov.
|
|
@cpojer when can we see this released in a new npm version? |
|
@jeanlauliac In my testing of the hack, I had to replace slashes in multiple places, not just the name variable. |
|
@patroza do you know which location needs additional transformation of the code? You referred some lines in the comment, but I'm not sure in which file it is. Would you be able to test with the new version of code from this PR, and if it misses some stuff, send a new PR? 😊 I have limited ability to reproduce unfortunately because I don't have a Windows machine to test with at the moment. Thank you so much! |
|
@jeanlauliac posted at #4018 |
* jest-haste-map: watchman crawler: normalize paths * also normalize path coming from watcher * simplify, add module + tests * add posix case
|
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. |
See the inline comment. New versions of Watchman on Window are now returning paths using forward slashes, but consumers of
jest-haste-mapuse to work and assume backslashes would be used (because they rely onpath.sep, that is backslash on win32, along the other path functions).This changeset make it so we
path.normalizethe paths we read from watchman. On POSIX systems this should not make any difference, but on Window this will have the effect of replacing forward slashes by the usual backslashes.