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

Ensure bundle:js command is not executed if clean fails #5820

Merged
merged 1 commit into from
Jun 7, 2023

Conversation

fluiddot
Copy link
Contributor

Following the changes from WordPress/gutenberg#51103, I realized that if for any reason (like a translations download failure) the clean command fails the bundle is still generated. We should ensure that the project is in a clean slate before generating the bundle, so I'd suggest aborting bundle:js command if clean fails.

To test:

  1. This can be tested by introducing an error in the clean npm command:
diff --git forkSrcPrefix/package.json forkDstPrefix/package.json
index 8e444e9dd3172fa1bf0549de3be8c7da09019c6c..116924d8c4f08d9ac9286d744c6a72003bd7c88a 100644
--- forkSrcPrefix/package.json
+++ forkDstPrefix/package.json
@@ -94,7 +94,7 @@
 		"test:e2e:ios:local": "npm run test:e2e:bundle:ios && npm run core test:e2e:build-app:ios && npm run core test:e2e:build-wda && TEST_RN_PLATFORM=ios npm run device-tests:local",
 		"sync:android": "bin/sync-android.sh",
 		"build:gutenberg": "cd gutenberg && npm run build",
-		"clean": "npm run core clean; npm run clean:gutenberg; npm run clean:i18n",
+		"clean": "exit 1; npm run core clean; npm run clean:gutenberg; npm run clean:i18n",
 		"clean:gutenberg": "cd gutenberg && npm run clean:packages",
 		"clean:gutenberg:distclean": "cd gutenberg && npm run distclean",
 		"clean:i18n": "rm -rf src/i18n-cache && npm run i18n:check-cache",
  1. Run the command npm run bundle.
  2. Observe that the command is aborted and that the bundle is not generated.
  3. Remove the above patch and run npm run bundle again.
  4. Observe that the bundle is generated successfully.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 31, 2023

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@geriux geriux self-requested a review June 1, 2023 12:26
Copy link
Contributor

@geriux geriux left a comment

Choose a reason for hiding this comment

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

Nice catch! This change makes sense, I have followed the testing instructions and it is working as expected. Nice work 🚀

I had to bring the changes from #5819 to be able to run the bundle script.

@fluiddot fluiddot enabled auto-merge June 1, 2023 13:59
@jhnstn
Copy link
Member

jhnstn commented Jun 7, 2023

Hey @fluiddot . We will cut the 1.97.0 release on Thursday 08, 2023. I plan to circle back and bump this PR to the next milestone then, but please let me know if you’d rather us work to include this PR in 1.97.0. Thanks!

@fluiddot
Copy link
Contributor Author

fluiddot commented Jun 7, 2023

Hey @fluiddot . We will cut the 1.97.0 release on Thursday 08, 2023. I plan to circle back and bump this PR to the next milestone then, but please let me know if you’d rather us work to include this PR in 1.97.0. Thanks!

Thanks for the ping @jhnstn 🙇 ! The PR is approved and should have been already merged but seems a failure in one of the CI jobs prevented it. I'll restart the job and check it finally gets merged today.

@fluiddot fluiddot merged commit b953534 into trunk Jun 7, 2023
@fluiddot fluiddot deleted the update-bundle-npm-command branch June 7, 2023 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants