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

Don't rebuild during the release process #12263

Merged
merged 1 commit into from
Oct 28, 2024
Merged

Don't rebuild during the release process #12263

merged 1 commit into from
Oct 28, 2024

Conversation

jjspace
Copy link
Contributor

@jjspace jjspace commented Oct 28, 2024

Description

The past couple releases we've had some extra .map.js files bleed through into the published version. These cause warnings for users downstream. See #12182

It seems these are generated when we run npm run test even with the --release option. I also noticed these would be generated any time we run npm start without --production as the dev server does a build when it starts up.

This PR adds a check for the release flag to skip building for npm run test. It also updates the release guide to specify the dev server should only be started with the --production flag.

One extra precaution we could add is a prepublishOnly lifecycle script to our package.json that calls npm run make-zip right before publish. Would make the publish command slower but guarantee we always build the version of files we want published.

Issue number and link

Fixes #12182

Testing plan

  • Run git clean -dxf to remove all extra files
  • Run npm run make-zip like you would during the release
  • Run npm run test -- --failTaskOnError --release like in the release guide
  • Make sure there are no messages about building and no .map.js files generated

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @jjspace!

✅ We can confirm we have a CLA on file for you.

@ggetz
Copy link
Contributor

ggetz commented Oct 28, 2024

Awesome, thanks @jjspace!

One extra precaution we could add is a prepublishOnly lifecycle script to our package.json that calls npm run make-zip right before publish. Would make the publish command slower but guarantee we always build the version of files we want published.

We can follow up about this in a separate thread when appropriate. But the only concern I have with this is that it means the versions of each of the files we just tested will not be the ones that get published. Perhaps a simpler measure would be to add the maps to .npmignore?

@ggetz ggetz merged commit e168f90 into main Oct 28, 2024
10 checks passed
@ggetz ggetz deleted the release-tests branch October 28, 2024 20:26
@jjspace
Copy link
Contributor Author

jjspace commented Oct 28, 2024

Perhaps a simpler measure would be to add the maps to .npmignore?

The problem with this when coupled with our current build process is that the flow when maps are generated is slightly different than the flow when they are not. If we just ignored the map files it could hide the fact that the files were rebuilt differently than we may want them to be.

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.

Sourcemap of Cesium.js (iife) points to missing source files (v1.121.0)
2 participants