Skip to content
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

Merged
merged 40 commits into from
Sep 13, 2019
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
08b6744
First iteration of the observer CLI
kaelig Sep 10, 2019
44da583
Load .ts files too
kaelig Sep 10, 2019
9912178
Add graph script
Sep 11, 2019
9d511ba
Enable compilation of .ts files
kaelig Sep 11, 2019
3adc842
Remove tinycolor2 dependency
kaelig Sep 11, 2019
a9da56e
Make filter more portable across environments
kaelig Sep 11, 2019
be17151
Add scoping to getGitStagedFiles
kaelig Sep 11, 2019
b0c0503
Try to use functions from treebuilder.ts
kaelig Sep 11, 2019
90801ff
Add types
Sep 11, 2019
5fde385
Fix filter
kaelig Sep 11, 2019
acd9c2e
Formatting
Sep 11, 2019
370d844
Remove absolute part of the path
kaelig Sep 11, 2019
8cb4113
Plug data from git status and improve listing
kaelig Sep 11, 2019
f7bfb82
Rename script to yarn splash, add watcher mode
kaelig Sep 12, 2019
61b233e
Align summary text to the right
kaelig Sep 12, 2019
e50011a
Change filtering
Sep 12, 2019
204bb60
Add temporary workaround to the skipIndexFile function
kaelig Sep 12, 2019
7ce1a5e
Move treebuilder.ts to scripts/splash
kaelig Sep 12, 2019
f10ffd3
Merge branch 'ink-cli' of github.com:Shopify/polaris-react into ink-cli
kaelig Sep 12, 2019
7493e70
Remove skipIndexFile
kaelig Sep 12, 2019
fb5019b
Add docs
kaelig Sep 12, 2019
0db5a4b
Add changelog item
kaelig Sep 12, 2019
72bc4ac
Merge branch 'master' of github.com:Shopify/polaris-react into ink-cli
kaelig Sep 12, 2019
230d07c
Add mention that the watcher is still alpha
kaelig Sep 12, 2019
961e3ae
Remove unused type dependency
kaelig Sep 12, 2019
4028392
Add missing space
kaelig Sep 12, 2019
af429e9
Add missing space
kaelig Sep 12, 2019
f397782
Verifies the index file is in a component folder
kaelig Sep 12, 2019
9d1873c
Account for .ts files
kaelig Sep 12, 2019
9b081de
Show utils.tsx files again
kaelig Sep 12, 2019
3502aef
Handle loading status properly
kaelig Sep 12, 2019
4a53045
Stop filtering index.ts files out
kaelig Sep 12, 2019
aed6ff3
Integrate with storybook
Sep 12, 2019
ec0fb8c
Make component grid layout responsive
kaelig Sep 12, 2019
12e60e6
Merge branch 'ink-cli' of github.com:Shopify/polaris-react into ink-cli
kaelig Sep 12, 2019
4a575dd
Delete watch mode
kaelig Sep 12, 2019
7dd8e0a
Update useEffects flow
Sep 12, 2019
7eeba2a
enum
Sep 13, 2019
768957f
Revert "enum"
kaelig Sep 13, 2019
1863788
Revert "Update useEffects flow"
kaelig Sep 13, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions UNRELEASED.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f

### Development workflow

- Added [`yarn splash` (beta)](/scripts/splash/), a command-line interface to observe the splash zone of a change across the component library ([#2113](https://github.com/Shopify/polaris-react/pull/2113))

### Dependency upgrades

### Code quality
Expand Down
11 changes: 8 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@
"readme-update-version": "node ./scripts/readme-update-version",
"version": "yarn run readme-update-version",
"storybook": "start-storybook -p 6006 --quiet",
"storybook:build": "yarn run copy-polaris-tokens && build-storybook -o build/storybook/static"
"storybook:build": "yarn run copy-polaris-tokens && build-storybook -o build/storybook/static",
"splash": "babel-node --extensions '.tsx','.ts' ./scripts/splash/index.tsx"
},
"stylelint": {
"extends": [
Expand Down Expand Up @@ -89,6 +90,7 @@
],
"devDependencies": {
"@babel/core": "^7.4.3",
"@babel/node": "^7.6.1",
"@percy/storybook": "^3.0.2",
"@shopify/jest-dom-mocks": "^2.1.1",
"@shopify/react-testing": "^1.7.3",
Expand All @@ -104,7 +106,8 @@
"@types/enzyme": "^3.10.3",
"@types/enzyme-adapter-react-16": "^1.0.5",
"@types/lodash": "^4.14.108",
"@types/node": "^11.13.8",
"@types/node": "^12.7.4",
kaelig marked this conversation as resolved.
Show resolved Hide resolved
"@types/tinycolor2": "^1.4.2",
"archiver": "^3.0.0",
"babel-core": "7.0.0-bridge.0",
"babel-loader": "^8.0.5",
Expand All @@ -118,9 +121,10 @@
"enzyme": "^3.10.0",
"enzyme-adapter-react-16": "^1.14.0",
"fs-extra": "^7.0.1",
"glob": "^7.1.3",
"glob": "^7.1.4",
"gray-matter": "^4.0.2",
"in-publish": "^2.0.0",
"ink": "^2.3.0",
"isomorphic-fetch": "^2.2.1",
"js-yaml": "^3.13.1",
"marked": "^0.6.2",
Expand Down Expand Up @@ -189,6 +193,7 @@
"@types/react-transition-group": "^2.0.7",
"hoist-non-react-statics": "^3.3.0",
"lodash": "^4.17.4",
"node-cmd": "^3.0.0",
"tslib": "^1.9.3"
}
}
6 changes: 6 additions & 0 deletions scripts/splash/.eslintrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"rules": {
"jsx-a11y/accessible-emoji": "off",
"shopify/jsx-no-hardcoded-content": "off"
}
}
23 changes: 23 additions & 0 deletions scripts/splash/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# `yarn splash` (beta)

`yarn splash` is a command-line interface to observe the splash zone of a change across the component library.

It answers the question:

> When I modify a component, what parts of the system did that also touch (also known as the change’s “splash zone”)?

## How to use `yarn splash`

1. Edit files in `src/`, such as components 🧩 and style sheets 🎨
2. Run `yarn splash` to see the splash zone of your changes in the working directory

## Tips

- <kbd>command</kbd> + click a file path to open it in your text editor
- `yarn splash --watch` continuously looks for changes in the working directory and updates the list of impacted files in real-time (alpha: you may need to hit <kbd>ctrl</kbd> + <kbd>c</kbd> multiple times to exit)

## Feedback and bug reports

Found an issue or want to share feedback?

Reach out on Slack in [#polaris-tooling](https://shopify.slack.com/messages/CCNUS0FML).
207 changes: 207 additions & 0 deletions scripts/splash/index.tsx
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) =>
Copy link
Contributor

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 checks

Copy link
Contributor Author

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.

!fileName.includes('test') &&
!fileName.includes('types') &&
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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))
Copy link
Contributor

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 a utils.ts file, I would still expect the tool to tell me accurately what the impact is

Copy link
Contributor Author

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.

.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
Copy link
Contributor

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

Copy link
Contributor Author

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.

.dirname(dependency)
.replace('src/components/', '')
.split('/')[0],
})),
['pathname', 'filename'],
),
}));

const Component = ({pathname, filename, dependencies}) => (
Copy link
Member

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 in Unknown component which was inside Unknown component.

<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 justifyContent="flex-end" width={3}>
{componentsModified}
</Box>
</Box>
<Box>
<Box width={30}>
<Text>Files potentially affected:</Text>
</Box>
<Box justifyContent="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');
Copy link
Contributor

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 prop

Copy link
Contributor Author

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.

Copy link
Member

@BPScott BPScott Sep 12, 2019

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

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[];
Copy link
Member

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 here

setStagedFiles(staged);
if (staged.length === 0) {
Copy link
Contributor

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?

Copy link
Contributor Author

@kaelig kaelig Sep 12, 2019

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.

Copy link
Contributor

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 on stagedFiles changing?

Copy link
Contributor Author

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.

Copy link
Contributor

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 themselves

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :/

setDataStatus('loaded');
}
};

useEffect(() => {
if (!argv.watch) {
getStagedFiles();
}
}, []);
// ^ empty dependency array = exits after one run

useEffect(() => {
if (argv.watch) {
getStagedFiles();
}
});
// ^ no dependencies = keeps watching
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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!

Copy link
Contributor

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

Copy link
Contributor Author

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 :shipit: and improve it later.


useEffect(() => {
if (stagedFiles.length > 0) {
const dependencies = getDependencies(
Copy link
Contributor

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?

Copy link
Contributor Author

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.

'src/**/*.tsx',
'*.test.tsx',
stagedFiles,
);
setData(formatDependencies(dependencies));
setDataStatus('loaded');
Copy link
Contributor

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?

Copy link
Contributor Author

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

}
}, [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 />);
Loading