-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Add typeahead to watch mode #2324
Conversation
Lol I literally just saw your comment on the issue and there is already a PR. Oh man. Let me see… |
Ok, so this is really cool! You did point out the question about how expensive creating an instance of However, we implemented file-watching in Originally I was thinking the watch mode should be its own separate package but I don't think we need to go that far. If we split out the watch mode into a separate file within So, to lay out the steps:
See https://github.com/facebook/react-native/blob/master/packager/react-packager/src/node-haste/index.js#L211-L216 for how the watch feature is used in react-native's packager. After that we can consider:
Again, I'd like to invite you to our discord channel ( http://facebook.github.io/jest/support.html ) and I'm also happy to have a chat over google hangouts or something to walk you through in person. Let me know what you think about this plan! I love that you started on this and this refactor is going to make Jest a ton faster for engineers at FB :) |
Current coverage is 66.19% (diff: 85.24%)@@ master #2324 diff @@
==========================================
Files 125 122 -3
Lines 4871 4840 -31
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 3157 3204 +47
+ Misses 1714 1636 -78
Partials 0 0
|
Thanks a lot, this feedback is super helpful, I'll definitely reach out if needed :) |
Alright, now that watch mode was rewritten, I believe we can pretty easily make this work with a great experience! Some thoughts:
|
I'm a big fan of highlighting search phrases. I believe yellow bg with dark text would be more than enough here, just like with Ctrl+F on Chrome |
That may work well, yeah! The other option I can think of is dim vs reset for matches, which is more subtle. |
👍 for the highlighting option. The dimming is clean but a bit too subtle I think. Also not sure how it would look on a light terminal background |
I would personally prefer the less aggressive dimming. Chrome does highlighting because you need to find these words between other words on the page. In this case, all results are displayed below, and you don’t really need to interact with them anyway. I’m not convinced we need to use a bright color here but YMMV. |
I agree with @gaearon. I think dimming is fine for this. |
I'm also happier with dimming/highlighting text instead of background highlight 🙂 |
Alright, @rogeliog let's go with the dimming for now and see how people react to the next release. |
const typeahead = (hasteMap, patternInfo) => { | ||
return new SearchSource(hasteMap, config) | ||
.getTestPaths(patternInfo) | ||
.then(data => { |
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.
these should be indented by two spaces.
return foo
.then(…)
let typeaheadLinesToClear = 0; | ||
|
||
const typeahead = (hasteMap, patternInfo) => { | ||
return new SearchSource(hasteMap, config) |
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.
do we need to create a new SearchSource every time the user types? I don't know how slow it is
if (!currentPattern) { | ||
return []; | ||
} else if (total === 0) { | ||
return ['No matching files...']; |
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.
Can you find of a good way to highlight this? Maybe yellow? Check out what happens when you do jest j23lk2j34l2k
on the repo, it will print a nice "No tests found" message, maybe we could align these two things.
Generated by 🚫 dangerJS |
Alright I think this looks good. The one problem I noticed is when my terminal output pushes me down to the bottom of the screen because of some failures. Typing feels a bit odd then, not sure if we can fix that? Shall we wipe away everything on the screen when entering pattern mode? @rogeliog was there anything else you wanted to do on this diff? |
|
||
hasteMap.on('change', ({hasteFS, moduleMap}) => { | ||
hasteContext = createHasteContext(config, {hasteFS, moduleMap}); | ||
currentPattern = ''; | ||
isEnteringPattern = false; |
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.
did you test this with file changes while you were typing?
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.
Yep, let me upload some gifs to show the issue
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.
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.
yup that's what I expected. Nice.
@cpojer I think it is in a good shape now, apart of the issue when there is not enough room in the terminal. We probably do need to find a good solution for it before merging, because it currently breaks really badly 😞 |
Do you have any ideas for that one? What about clearing the terminal when entering pattern mode? |
@cpojer Yeah I think we are going to have to do something like that. Here are some of my thoughts on this 1) Clear the terminal and only show pattern mode
I like how clean this option is but it might also be a bit confusing. We could add a
2) Clear the terminal and show both usage and pattern mode
This one might end up in a more familiar experience for current Jest users. Some issuesLosing test resultsOne problem I see with either of this solutions is that we lose the test results
Handle small terminalsRegardless of which one we choose we might still need to handle the case where the terminal height is less than the number of lines printed by pattern mode. Depending on which solution we choose this might not be as much of an issue. |
I think showing watch usage only when going into pattern mode might be the least complex and most sensible solution for now. Let's not worry too much about small terminals. |
|
||
if (pattern) { | ||
if (total) { | ||
pipe.write(`\n\n Pattern matches ${total} ${pluralizeFile(total)}...`); |
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.
Why the ...
here? IMO it's unnecessary.
Usually ...
is used in the UI on the buttons that present an additional dialog (e.g. Open...
).
In this case just .
will do.
@cpojer, @gaearon here a gif of the experience as of the latest commit in this PR After we decide to merge this, there are a couple polish items that I would like to focus on to make the typeahead experience as smooth as is should be. For example: Handling screen resizes by truncateing(...) the results appropiately and investigate a way allow the use of arrows... Also, I took some decisions on the copy for the "Pattern mode", feedback is more than welcome 😄 |
Let's do it. @rogeliog do you care to create a master issue (+ smaller issues referenced from it) to track further polish work? This is a really delightful experience and I'm looking forward to further improvements. |
So nice. |
* Extract typeahead printing logic * Handle escape when in typeahead * Add tests and highlight * Update Flow and fix flow errors. * Improve printing of test paths. * Polish * Fix node 4 CI * Reset pattern state if FS changes * Only instantiate searchSource once * Clear screen when entering typeahead * Show "pattern mode" and clear screen
* Extract typeahead printing logic * Handle escape when in typeahead * Add tests and highlight * Update Flow and fix flow errors. * Improve printing of test paths. * Polish * Fix node 4 CI * Reset pattern state if FS changes * Only instantiate searchSource once * Clear screen when entering typeahead * Show "pattern mode" and clear screen
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
@cpojer I found #2251 really interesting, so I started working on it just to get a bit more familiar with the internals of Jest, of which I know nothing.
This is a really primitive prototype, which still has several issues that need to be address, as well as testing.
This is the typeahead running in the React repo
In the issue you mentioned that there is some refactor that will make this easier to accomplish, so I guess that this might not be the right way of implementing it:
Assuming that we want to move forward with the feature here are a couple of things that I'm still missing
Runtime.createHasteContext
? Is it something that I should be caching?