Make yarn splash optional #2372

amrocha · 2019-10-28T18:31:47Z

WHY are these changes introduced?

Yarn splash can be really noisy at times

WHAT is this pull request doing?

Enables maintainers running yarn dev to hide yarn splash reports from the console by running DISABLE_SPLASH=1 yarn dev (#2372)

github-actions · 2019-10-28T18:37:30Z

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

No significant changes to src/**/*.tsx were detected.

This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

kaelig · 2019-10-28T22:02:41Z

scripts/splash/index.tsx

+          <Box>
+            Tip: disable <Text bold>yarn splash</Text> by running{' '}
+            <Text bold>yarn dev-no-splash</Text> or setting an environment
+            variable <Text bold>DISABLE_SPLASH=1</Text>


Isn't it a bit strange to have this message here when the responsibility for this condition is handled by the caller? This means that running yarn splash will show this message.

People need to learn about it somehow or we're always going to get messages from people like "hey how do I turn off yarn splash". Even if we let people on Slack know, there's outside contributors and new people joining all the time.

I see what you mean about this showing up when people run the standalone command. I can make it show up when storybook is running, but not when it's run by itself. How about that?

I totally agree that showing a message is good for awareness 👍

My question was around whether it's appropriate to have the message living in scripts/splash/, when the code that handles what's advertised by the message lives elsewhere (in the webpack config).

Alternatively, would it make more sense to handle the conditional for DISABLE_SPLASH in scripts/splash/index.ts, so both the message and the condition live under the same roof?

I tried doing it in the webpack config but webpack overrides stdout so I can't just echo.

I could create a separate script for it but I think that would be adding too much complexity for very little gain

kaelig · 2019-10-28T22:03:30Z

package.json

    "prepublish": "in-publish && yarn run build || :",
    "postpublish": "in-publish && npm-run-all new-version-pr-generator",
    "dev": "npm-run-all copy-polaris-tokens storybook",
+    "dev-no-splash": "DISABLE_SPLASH=1 yarn run dev",


Not sure this warrants adding a new script – it might be okay to tell people to run DISABLE_SPLASH=1 yarn run dev?

This is the same reason as above, I wanted to document it so that people can find the option easily.

If we don't add a command, how many people are going to dig into the code to figure out they can set an environment variable? Probably not a lot

The console message and Splash's README do a great job telling people about the DISABLE_SPLASH trick.

I see your point about adding even more discoverability, however having 2 ways of achieving the same thing means we have to document and provide support/answer questions about both.

What's the support we need to provide for a yarn command? Seems fairly straightforward to me

What's the support we need to provide for a yarn command?

When yarn splash fails or needs to be disabled for any reason, we'd be giving people 3 ways of disabling it:

DISABLE_SPLASH=1 yarn dev

DISABLE_SPLASH=1 environment variable + yarn dev

yarn dev-no-splash (probably doesn't work in Windows, btw)

It's not a lot of overhead, but it adds to the overall cognitive load for contributors.

Also, if people start using yarn dev-no-splash and we want to remove it later, we'll be disrupting their developer experience (and I really don't want us to have to think about a deprecation path for custom yarn scripts).

Plus, that's less documentation for us to write and keep up to date 🎉

PS: I agree with the intent behind having it more discoverable, but I'm not sure it's appropriate to do it that way, especially since the console message & README doc are great ways to advertise this feature.

kaelig · 2019-10-28T22:03:57Z

.storybook/webpack.config.js

-          shell: true,
-          stdio: 'inherit',
-        });
+        process.env.DISABLE_SPLASH ||


The environment variable is a great idea 👍

kaelig · 2019-10-28T22:10:05Z

UNRELEASED.md

 ### Development workflow

 - Removed the need to upload assets with each release ([#2346](https://github.com/Shopify/polaris-react/pull/2346))
+- Made running yarn splash along with the storybook optional ([#2372](https://github.com/Shopify/polaris-react/pull/2372))


We could save people a click by telling them a bit more about this.

Here's a suggestion:

Suggested change

- Made running yarn splash along with the storybook optional ([#2372](https://github.com/Shopify/polaris-react/pull/2372))

- Made it possible to prevent [“splash”](https://github.com/Shopify/polaris-react/tree/master/scripts/splash) from automatically running alongside `yarn dev`: `DISABLE_SPLASH=1 yarn dev` ([#2372](https://github.com/Shopify/polaris-react/pull/2372))

kaelig

A few questions and suggestions – will 🎩 as a second pass when those are answered.

amrocha · 2019-10-28T22:22:59Z

The reason I made it this way is discoverability.

I think we should keep the message in there for sure. I'd prefer to keep the command in there too, since the storybook output is very noisy and I think people might not find it, but this is redundant if we have the message in there so I'm ok with removing it if you want to.

dleroux

Works great, thanks for doing this @amrocha!

I do think the script inside the package json was a good idea. It's a natural place to look for that sort of thing. Not a blocker, just my 2 cents.

kaelig · 2019-11-05T00:37:04Z

Hey @dleroux, I just merged a3354b8 into this branch to contain the logic into the splash script itself. Can you give this an extra review?

dleroux · 2019-11-05T12:48:48Z

scripts/splash/README.md

 1. Edit files in `src/`, such as components 🧩 and style sheets 🎨.
 2. As you run `yarn dev`, `yarn splash` will run in the background. Keep an eye on the terminal to see the splash zone of your changes in the working directory.

+   💡 Tip: to disable these reports, run `DISABLE_SPLASH=1 yarn dev`


Did you mean to indent this tip?

Yep 👍 (it's nested under the list item)

UNRELEASED.md

amrocha requested a review from dleroux October 28, 2019 18:32

kaelig reviewed Oct 28, 2019

View reviewed changes

dleroux approved these changes Oct 30, 2019

View reviewed changes

amrocha added 5 commits October 30, 2019 14:30

Make yarn splash optional

4af2878

Add changelog

7c900e7

Make message only show up when running as part of storybook

f43f112

Rename flag, add tip to readme, remove package json script

4f1b494

Add command again, update changelog wording

f5f4a78

amrocha force-pushed the make-splash-optional branch from c644e4b to f5f4a78 Compare October 30, 2019 21:30

[yarn splash] DISABLE_SPLASH redux (#2398)

a3354b8

kaelig requested a review from dleroux November 5, 2019 00:35

dleroux reviewed Nov 5, 2019

View reviewed changes

UNRELEASED.md Outdated Show resolved Hide resolved

kaelig added 3 commits November 6, 2019 15:01

Update UNRELEASED.md

9a5b018

Update UNRELEASED.md

b58d9fe

Update UNRELEASED.md

00a1db4

kaelig merged commit 63981fe into master Nov 6, 2019

kaelig deleted the make-splash-optional branch November 6, 2019 23:06

	- Made running yarn splash along with the storybook optional ([#2372](https://github.com/Shopify/polaris-react/pull/2372))
	- Made it possible to prevent [“splash”](https://github.com/Shopify/polaris-react/tree/master/scripts/splash) from automatically running alongside `yarn dev`: `DISABLE_SPLASH=1 yarn dev` ([#2372](https://github.com/Shopify/polaris-react/pull/2372))

Make yarn splash optional #2372

Make yarn splash optional #2372

Uh oh!

Conversation

amrocha commented Oct 28, 2019 • edited by kaelig Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

Uh oh!

github-actions bot commented Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaelig Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaelig left a comment

Choose a reason for hiding this comment

Uh oh!

amrocha commented Oct 28, 2019

Uh oh!

dleroux left a comment

Choose a reason for hiding this comment

Uh oh!

kaelig commented Nov 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

amrocha commented Oct 28, 2019 •

edited by kaelig

Loading

github-actions bot commented Oct 28, 2019 •

edited

Loading

kaelig Oct 29, 2019 •

edited

Loading