[Tooling] yarn splash (beta)
#2113
Conversation
kaelig
changed the title
yarn splash (beta)
|
|
||
| const formatDependencies = (dependencies) => | ||
| dependencies | ||
| .filter(({fileName}) => excludedFileNames(fileName)) |
There was a problem hiding this comment.
Do you think we should filter here too?
I think, for example, if you made a change to Avatar/index.ts, or to a utils.ts file, I would still expect the tool to tell me accurately what the impact is
There was a problem hiding this comment.
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.
scripts/splash/index.tsx
Outdated
|
|
||
| const excludedFileNames = (fileName) => | ||
| !fileName.includes('test') && | ||
| !fileName.includes('types') && |
There was a problem hiding this comment.
I think we'll want to include types in the future, when we have per-export granularity. Right now I'm of two minds about it, maybe we should include types and maybe we shouldn't. I'll leave it up to you
There was a problem hiding this comment.
IMHO type changes are really well handled via VSCode, so I'm not sure we'd be adding value by having this in the tool.
| import {argv} from 'yargs'; | ||
| import {getGitStagedFiles, getDependencies} from './treebuilder'; | ||
|
|
||
| const excludedFileNames = (fileName) => |
There was a problem hiding this comment.
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 checks
There was a problem hiding this comment.
Yeah I agree. As we make it more codebase-agnostic, this won't scale.
scripts/splash/index.tsx
Outdated
| const staged = (await getGitStagedFiles('src/')) as string[]; | ||
| setStagedFiles(staged); | ||
|
|
||
| if (staged.length === 0) { |
There was a problem hiding this comment.
I'm not sure I understand this, can you explain why you have this check?
There was a problem hiding this comment.
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.
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 on stagedFiles changing?
There was a problem hiding this comment.
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.
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 themselves
There was a problem hiding this comment.
move the state handling part to the hooks themselves
I tried that, but then it ended up "watching" in both useEffects, instead of the first one only running once and then exiting :/
scripts/splash/index.tsx
Outdated
| getStagedFiles(); | ||
| } | ||
| }); | ||
| // ^ no dependencies = keeps watching |
There was a problem hiding this comment.
Why not just use this one?
There was a problem hiding this comment.
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.
So does this just constantly execute in the background?
There was a problem hiding this comment.
Yeah, not sure how, though!
There was a problem hiding this comment.
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.
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.
|
|
||
| useEffect(() => { | ||
| if (stagedFiles.length > 0) { | ||
| const dependencies = getDependencies( |
There was a problem hiding this comment.
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.
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.
| const App = () => { | ||
| const [stagedFiles, setStagedFiles] = useState([]); | ||
| const [data, setData] = useState([]); | ||
| const [dataStatus, setDataStatus] = useState('loading'); |
There was a problem hiding this comment.
I think it makes more sense to make this a loading boolean prop
There was a problem hiding this comment.
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.
In that case can we have the vale be an enum rather than an arbitary string. There's an example of that in #2067
enum Status {
Pending = 'PENDING',
Loaded = 'LOADED',
Errored = 'ERRORED',
}Then in your useState:
const [status, setStatus] = useState<Status>(Status.Pending);| stagedFiles, | ||
| ); | ||
| setData(formatDependencies(dependencies)); | ||
| setDataStatus('loaded'); |
There was a problem hiding this comment.
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.
Not sure what you mean by that, but let's talk about it during our next pairing session
There was a problem hiding this comment.
I've not got much skin in this game and this is a first pass so whatevs.
Code in the treebuilder looks good. It's pretty dense so I'm not going to pretend to understand it but does the job. It looks like we might run into issues if we have circular dependencies, I'd be nice to work out how to stop that, but it doesn't need to block getting this merged.
I've got some concerns with the display rendering:
- Currently it truncates content if if text is thinner than your terminal and we're all been around the web long enough to know truncation is not a content strategy. You can see this in action if you go see the results of editing
src/components/ResourceList/components/FilterControl/components/DateSelector/Da teSelector.tsx - The output does fancy writing into placeholder area things, I don't know how that redrawing of the terminal will work if you want to output info this as part of the storybook webpack build, but my gut fears this may be too clever.
It'd also be really nice to be able to specify a filepath / glob to check on the command line so I don't have to go edit a particular file to see its splashzone. Checking changed items can be done with a flag:
yarn run splash src/components/Button/Button.tsx shows you button's splashzone, while yarn run splash --modified (or some other arg) will show you the output of changed items
| ), | ||
| })); | ||
|
|
||
| const Component = ({pathname, filename, dependencies}) => ( |
There was a problem hiding this comment.
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 in Unknown component which was inside Unknown component.
scripts/splash/index.tsx
Outdated
| const [dataStatus, setDataStatus] = useState('loading'); | ||
|
|
||
| const getStagedFiles = async () => { | ||
| const staged = (await getGitStagedFiles('src/')) as string[]; |
There was a problem hiding this comment.
Can we fix this by setting the return type of getGitStagedFiles to be Promise<string[]> in treebuilder instead of having to redefine it here
| } | ||
| } | ||
|
|
||
| return Object.keys(dependencies) |
There was a problem hiding this comment.
This feels like an opinion on output formatting, should this filtering of index files be the responsibility of the rendering logic in splash/index?
| compile(codebase, { | ||
| noEmitOnError: true, | ||
| noImplicitAny: true, | ||
| target: ts.ScriptTarget.ES5, |
There was a problem hiding this comment.
Do these output config bits need to be configured if we're ingesting the code but not outputting anything from TS?
Screenshots
First iteration
Second iteration (with real data)
Third iteration (loading state, empty state)
No changes detected
Loading
Changes detected