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

Update minification and sourcemap settings on CI builds for sentry #19583

Merged
merged 9 commits into from
Nov 3, 2021

Conversation

novocaine
Copy link
Contributor

@novocaine novocaine commented Nov 2, 2021

With previous settings, our JS files for develop are so large that sentry's webserver rejects the upload.

In this change -

  • re-enable minification to reduce the size of the files
  • update the CI sourcemap setting from eval-source-map to source-map to move the embedded source out of the .js payload and into .js.map files

This step fixed the issue for me locally at least by reducing the size of init.js from ~25MB to a 2MB js + 10MB .map

This PR also includes a careful refactor of the loading of build config depending on the args and environment, its best to follow this commit by commit.


This PR currently has no changelog labels, so will not be included in changelogs.

A reviewer can add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is, or add Type: [enhancement/defect/task] to the description and I'll add them for you.

@novocaine novocaine requested a review from a team as a code owner November 2, 2021 11:36
@novocaine novocaine requested a review from turt2live November 2, 2021 11:36
@novocaine novocaine force-pushed the enable-ci-minification-for-sentry-develop branch from 4dc9058 to ced0dd7 Compare November 2, 2021 16:17
@dbkr
Copy link
Member

dbkr commented Nov 2, 2021

Looks sensible. I can't see any reason un-minified sources would be useful if we ship the app with source maps anyway, whether or not we publish them to sentry - would be good to double check from others.

@novocaine novocaine changed the title Re-enable minification of CI builds for sentry Update minification and sourcemap settings on CI builds for sentry Nov 2, 2021
@novocaine
Copy link
Contributor Author

I realised that the problem is actually the embedded source is huge, so trying with source-map which splits the embedded source into a different .map file. Not really sure whether our deployment script will upload .map files - I think it just bindly unpacks the tar file?

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

otherwise this seems fine - developer machines won't explode, and seems to result in better bundles for end users. Assuming Sentry works with this, let's do it.

const devMode = nodeEnv !== 'production';
const useHMR = process.env.CSS_HOT_RELOAD === '1' && devMode;
const fullPageErrors = process.env.FULL_PAGE_ERRORS === '1' && devMode;
const enableMinification = argv.mode === "production";
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer if we instead used !devMode or otherwise relied upon nodeEnv as a variable

@novocaine novocaine enabled auto-merge (squash) November 3, 2021 09:49
@novocaine novocaine merged commit d9f72ec into develop Nov 3, 2021
@novocaine novocaine deleted the enable-ci-minification-for-sentry-develop branch November 3, 2021 09:55
novocaine added a commit that referenced this pull request Nov 3, 2021
novocaine added a commit that referenced this pull request Nov 3, 2021
novocaine added a commit that referenced this pull request Nov 3, 2021
novocaine added a commit that referenced this pull request Nov 3, 2021
* Revert "Revert "Update minification and sourcemap settings on CI builds for sentry (#19583)" (#19601)"

This reverts commit 516e38c.

* Disable minification in CI as it exceeds memory limits for poor buildkite
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