-
Notifications
You must be signed in to change notification settings - Fork 56
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
Update npm run start
to include cleanup
#2970
Conversation
Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job! |
2d2c847
to
50a2688
Compare
Helps to avoid unexpected bugs that can arise when using the old version of `npm run start`. Now devs will need to explicitly choose to use the `npm run start:quick` command to get the old, riskier, `npm run start` behavior.
50a2688
to
66e576d
Compare
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 a blocker at all but I was thinking that could be interesting to add some styling to the message although styling in Bash is probably not the most beautiful code 😄 .
The following line would produce:
"start": "echo \"\\x1b[33mThe start command is not available in this project. It is strongly recommended to use \\x1b[1:33mstart:reset\\x1b[0m\\x1b[33m to perform some cleanup when starting the metro bundler.\nOr you may use \\x1b[1:33mstart:quick\\x1b[0m\\x1b[33m for a quicker startup, but this may lead to unexpected javascript errors when running the app.\\x1b[0m\"",
Another option would be to add a simple script and use util functions like the ones we have in the release-toolkit-gutenberg-mobile
repository.
Apart from that I tested the commands and work nice 💯 !
After checking the description and the code I found that they don't match, |
You're absolutely right, my testing instructions had the two commands reversed there. Thanks for catching that (I've updated the instructions). |
Good idea, and thanks for the suggested code @fluiddot ! I made that change both here and in the gutenberg PR (which still needs a review 😉 ). |
I'd also change the first command of this part because Ensure 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.
LGTM! I tested the commands and works fine.
PD: I added a comment about the PR's description because it doesn't match the code changes.
👋 @mchowning it looks like this was already mentioned in this comment, but was it intentional that in the terminal (Bash) the output shows raw formatting characters? It's only when I looked further down that I see the cleaner version of the message.
|
Not really intentional @guarani , more just that npm seems to print the script itself (in this case the
If there's an easy way to prevent npm from printing the script, it might be a nice improvement. |
By default it prints the executed command, for not printing it there's an option in NPM named
|
Sorry @mchowning, intentional wasn't the right word there. I meant to ask if it was a known issue. I'm normally in bash and the bold formatting didn't come through. I switched to |
I'm using |
👋 I'm moving this to Long story short, a PR was merged into the WordPress Android app after the Let me know if there's any concern about including in the beta fix, thanks! |
Related gutenberg PR: WordPress/gutenberg#28025
Helps to avoid unexpected bugs that could arise if you used the old version of
npm run start
instead ofnpm run start:reset
. Now the cleanup instart:reset
is included instart
. Devs who want to start the bundler without the cleanup will need to explicitly use thenpm run start:quick
command.To test:
Ensure that
npm run start:reset
andnpm run start:quick
both start the metro bundler. Also check thatnpm run start
outputs an error message directing the user to thenpm run start:reset
command.PR submission checklist: