-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Changes from 1 commit
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
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
# `yarn splash` | ||
|
||
A command-line interface to observe the splash zone of a change across the component library. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,207 @@ | ||
import path from 'path'; | ||
import React, {useState, useEffect} from 'react'; | ||
import {Box, Text, Color, render} from 'ink'; | ||
import sortBy from 'lodash/sortBy'; | ||
import {argv} from 'yargs'; | ||
import {getGitStagedFiles, getDependencies} from '../../treebuilder'; | ||
|
||
const excludedFileNames = (fileName) => | ||
!fileName.includes('test') && | ||
!fileName.includes('types') && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
!fileName.endsWith('index.ts') && | ||
kaelig marked this conversation as resolved.
Show resolved
Hide resolved
|
||
!fileName.endsWith('utils.tsx'); | ||
kaelig marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const getEmojiForExtension = (extension) => { | ||
switch (extension) { | ||
case '.tsx': | ||
kaelig marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return '🧩'; | ||
case '.scss': | ||
return '🎨'; | ||
default: | ||
return '❔'; | ||
} | ||
}; | ||
const formatDependencies = (dependencies) => | ||
dependencies | ||
.filter(({fileName}) => excludedFileNames(fileName)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
.map((dependency) => ({ | ||
pathname: `${path.dirname(dependency.fileName)}/`, | ||
filename: path.basename(dependency.fileName), | ||
dependencies: sortBy( | ||
dependency.dependencies.filter(excludedFileNames).map((dependency) => ({ | ||
pathname: `${path.dirname(dependency)}/`, | ||
filename: path.basename(dependency), | ||
componentName: path | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I'll have to think about those edge cases, yeah. |
||
.dirname(dependency) | ||
.replace('src/components/', '') | ||
.split('/')[0], | ||
})), | ||
['pathname', 'filename'], | ||
), | ||
})); | ||
|
||
const Component = ({pathname, filename, dependencies}) => ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please define these components (and really all top-level functions) using the |
||
<Box marginBottom={1} flexDirection="column"> | ||
<Box> | ||
<Box width={3}>{getEmojiForExtension(path.extname(filename))}</Box> | ||
<Text bold> | ||
<Color greenBright> | ||
{pathname} | ||
{filename} | ||
</Color> | ||
</Text> | ||
</Box> | ||
<Box marginLeft={3}> | ||
<Box width={23}>Component name</Box> | ||
Files potentially affected (total: {dependencies.length}) | ||
</Box> | ||
{dependencies.map(({pathname, filename, componentName}) => ( | ||
<Box marginLeft={3} key={pathname + filename}> | ||
<Box width={23}> | ||
<Color dim>{'<'}</Color> | ||
<Color>{componentName}</Color> | ||
<Color dim>{' />'}</Color> | ||
</Box> | ||
<Text> | ||
<Color dim>{pathname}</Color> | ||
{filename} | ||
</Text> | ||
</Box> | ||
))} | ||
</Box> | ||
); | ||
|
||
const Components = ({components, status}) => ( | ||
<React.Fragment> | ||
{status === 'loading' && ( | ||
<Box marginLeft={4} marginBottom={1}> | ||
⏳{' '}Please wait during compilation… Beep boop beep 🤖 | ||
</Box> | ||
)} | ||
|
||
{status === 'loaded' && | ||
components.map(({pathname, filename, dependencies}) => ( | ||
<Component | ||
key={pathname + filename} | ||
pathname={pathname} | ||
filename={filename} | ||
dependencies={dependencies} | ||
/> | ||
))} | ||
</React.Fragment> | ||
); | ||
|
||
const Summary = ({ | ||
componentsModified, | ||
dependencies, | ||
}: { | ||
componentsModified: number; | ||
dependencies: number; | ||
}) => ( | ||
<Box flexDirection="column"> | ||
<Box> | ||
<Box width={30}> | ||
<Text>Files modified:</Text> | ||
</Box> | ||
<Box alignItems="flex-end" width={3}> | ||
{componentsModified} | ||
</Box> | ||
</Box> | ||
<Box> | ||
<Box width={30}> | ||
<Text>Files potentially affected:</Text> | ||
</Box> | ||
<Box alignItems="flex-end" width={3}> | ||
{dependencies > -1 ? dependencies : '⏳'} | ||
kaelig marked this conversation as resolved.
Show resolved
Hide resolved
|
||
</Box> | ||
</Box> | ||
</Box> | ||
); | ||
|
||
const App = () => { | ||
const [stagedFiles, setStagedFiles] = useState([]); | ||
const [data, setData] = useState([]); | ||
const [dataStatus, setDataStatus] = useState('loading'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it makes more sense to make this a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 enum Status {
Pending = 'PENDING',
Loaded = 'LOADED',
Errored = 'ERRORED',
} Then in your useState: const [status, setStatus] = useState<Status>(Status.Pending); |
||
|
||
const getStagedFiles = async () => { | ||
const staged = (await getGitStagedFiles('src/')) as string[]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we fix this by setting the return type of |
||
setStagedFiles(staged); | ||
if (staged.length === 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Right, I think it's fine to keep the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I tried that, but then it ended up "watching" in both |
||
setDataStatus('loaded'); | ||
} | ||
}; | ||
|
||
useEffect(() => { | ||
if (!argv.watch) { | ||
getStagedFiles(); | ||
} | ||
}, []); | ||
// ^ empty dependency array = exits after one run | ||
|
||
useEffect(() => { | ||
if (argv.watch) { | ||
getStagedFiles(); | ||
} | ||
}); | ||
// ^ no dependencies = keeps watching | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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. |
||
|
||
useEffect(() => { | ||
if (stagedFiles.length > 0) { | ||
const dependencies = getDependencies( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
'src/**/*.tsx', | ||
'*.test.tsx', | ||
stagedFiles, | ||
); | ||
setData(formatDependencies(dependencies)); | ||
setDataStatus('loaded'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You never set this prop to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
}, [setData, stagedFiles]); | ||
|
||
return ( | ||
<React.Fragment> | ||
<Box marginBottom={1} flexDirection="column"> | ||
<Box> | ||
<Box width={3}>💦</Box> | ||
<Box> | ||
<Text bold>yarn splash</Text>: Observe the splash zone of a change | ||
across the entire library | ||
</Box> | ||
</Box> | ||
</Box> | ||
<Components components={data} status={dataStatus} /> | ||
<Summary | ||
componentsModified={stagedFiles.length} | ||
dependencies={ | ||
dataStatus === 'loading' | ||
? -1 | ||
: new Map([ | ||
...data.map(({pathname, filename}) => [ | ||
pathname, | ||
pathname + filename, | ||
]), | ||
...data.reduce( | ||
(val, curr) => | ||
val.concat( | ||
curr.dependencies.map(({pathname}) => [ | ||
pathname, | ||
curr.pathname + curr.filename, | ||
]), | ||
), | ||
[], | ||
), | ||
]).size | ||
} | ||
/> | ||
<Box marginTop={1}> | ||
<Color dim> | ||
<Box width={3}>💡</Box> | ||
<Box> | ||
tip: command + click a file path to open it in your text editor | ||
</Box> | ||
</Color> | ||
</Box> | ||
</React.Fragment> | ||
); | ||
}; | ||
|
||
render(<App />); |
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.