Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Tooling]
yarn splash
(beta) #2113[Tooling]
yarn splash
(beta) #2113Changes from 32 commits
08b6744
44da583
9912178
9d511ba
3adc842
a9da56e
be17151
b0c0503
90801ff
5fde385
acd9c2e
370d844
8cb4113
f7bfb82
61b233e
e50011a
204bb60
7ce1a5e
f10ffd3
7493e70
fb5019b
0db5a4b
72bc4ac
230d07c
961e3ae
4028392
af429e9
f397782
9d1873c
9b081de
3502aef
4a53045
aed6ff3
ec0fb8c
12e60e6
4a575dd
7dd8e0a
7eeba2a
768957f
1863788
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think these checks are a bit too lose. For example,
.includes('test')
will also filter file names that just happen to include those characters, even if it's not a test. I think it'd be good to be a bit more specific here for all checksThere 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.
Yeah I agree. As we make it more codebase-agnostic, this won't scale.
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 you think we should filter here too?
I think, for example, if you made a change to
Avatar/index.ts
, or to autils.ts
file, I would still expect the tool to tell me accurately what the impact isThere 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.
You're right. Let me put those index and utils back in. If feedback says this is too overwhelming we can always filter them out again.
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 might not necessarily be a component
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.
I'll have to think about those edge cases, yeah.
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.
Please define these components (and really all top-level functions) using the
function Component()
style syntax. Using Arrow functions means that the components will all have no name to use for debugging which will be a pain when you get stack trace staying there was an error inUnknown
component which was insideUnknown
component.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.
I think it makes more sense to make this a
loading
boolean propThere 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.
Right now it's loading/loaded, but there can be other loading states, such as 'error' or 'cancelled' that I'd like to leave this open for.
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.
In that case can we have the vale be an enum rather than an arbitary string. There's an example of that in #2067
Then in your useState:
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 we fix this by setting the return type of
getGitStagedFiles
to be Promise<string[]> in treebuilder instead of having to redefine it hereThere 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.
I'm not sure I understand this, can you explain why you have this check?
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.
If there are no staged files, then we don't need to run a compilation task and we can stop updating the UI.
Setting the data status to 'loaded' is a way to do that.
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.
Logically this is a bit confusing, since this function is just supposed to get the staged files.
Can you move this to a
useEffect
hook that executes onstagedFiles
changing?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.
That's exactly what's happening: this is executed in two useEffect hooks hence why it's in its own function.
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.
Right, I think it's fine to keep the
getStagedFiles
function separate. But it doesn't make sense to set state in this function, that's why I think you should move the state handling part to the hooks themselvesThere 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.
I tried that, but then it ended up "watching" in both
useEffect
s, instead of the first one only running once and then exiting :/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 not just use this one?
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.
It doesn't exit correctly (promises keep firing). I kind of stumbled upon this by accident and thought it'd be a good way to have an alpha for the watcher until we have a better solution.
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.
So does this just constantly execute in the background?
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.
Yeah, not sure how, though!
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.
Hmm, my guess is that it's just re-running constantly. In that case can you run a quick perf check? If it's not a big impact then it's fine, but I want to avoid a situation where we're shipping something that slows down people's computers for no discernible reason
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.
As discussed: this is not good for performance, but not crippling.
Since it's behind a command argument and we tell people this is an alpha, let's and improve it later.
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.
I'm not too happy with the API here, what do you think? Any suggestions for how to improve it?
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.
I also think we can improve it. Since we'll gain insights as you explore how to make this work for GitHub, webpack, and storybook… I'd like to wait to revamp it when you know more about all these other use cases.
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.
You never set this prop to
loading
. Wouldn't you want to do that when a re-render is triggered, and you're re-running the script and the git status function?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.
Not sure what you mean by that, but let's talk about it during our next pairing session