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

fix(ember): Restore ember package contents #5318

Merged
merged 1 commit into from Jun 27, 2022

Conversation

lforst
Copy link
Member

@lforst lforst commented Jun 27, 2022

Problem Description

As reported in #5296, from version 7.1.1 to 7.2.0 of the Ember SDK, a majority of the contents of the npm packages suddenly disappeared.

We traced the issue back to a Node.js dependency change (in patch v16.15.1) that bumped npm from version 8.5.5 to 8.11.0. That npm version bump included a fix (npm/cli#4917 & npm/npm-packlist#102, first appeared in npm v8.11.0) that restores the functionality that npm pack also looks at workspace roots for .npmignore files. For the ember SDK package, we actually depended on the bug that is now fixed, as a result messing up our package contents.

Changes

To fix the ember package contents we simply override the workspace root .npmignore rules in the .npmignore file of the ember package. Essentially, for the ember package, this now works as if there were no .npmignore file in the workspace root.

Additionally, as of this PR, we pin the default node version (and implicitly the npm version). So we have reproducible builds.


Fixes #5296

@github-actions
Copy link
Contributor

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.3 KB (added)
@sentry/browser - ES5 CDN Bundle (minified) 59.72 KB (added)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 17.9 KB (added)
@sentry/browser - ES6 CDN Bundle (minified) 52.67 KB (added)
@sentry/browser - Webpack (gzipped + minified) 19.67 KB (added)
@sentry/browser - Webpack (minified) 64.04 KB (added)
@sentry/react - Webpack (gzipped + minified) 19.69 KB (added)
@sentry/nextjs Client - Webpack (gzipped + minified) 43.85 KB (added)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.64 KB (added)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 23.91 KB (added)

@lforst lforst requested review from Lms24 and AbhiPrasad June 27, 2022 09:06
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

LGTM. Given how different the ember package is from the rest, I think overriding the default .npmignore is definetily justified.

Just in case you didn't, let's make sure we inspect the created tarball contents before releasing the next patch.

@@ -58,15 +59,15 @@ jobs:
echo "COMMIT_SHA=$COMMIT_SHA" >> $GITHUB_ENV
echo "COMMIT_MESSAGE=$(git log -n 1 --pretty=format:%s $COMMIT_SHA)" >> $GITHUB_ENV
outputs:
commit_label: "${{ env.COMMIT_SHA }}: ${{ env.COMMIT_MESSAGE }}"
commit_label: '${{ env.COMMIT_SHA }}: ${{ env.COMMIT_MESSAGE }}'
Copy link
Member

Choose a reason for hiding this comment

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

No action required: I guess those are some automatic Prettier (or similar) changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, I'll leave it (I'm sorry for the messed up git history).

@lforst
Copy link
Member Author

lforst commented Jun 27, 2022

Just in case you didn't, let's make sure we inspect the created tarball contents before releasing the next patch.

before.txt
after.txt
diff.txt

I generated all of the tarballs locally before and after the changes in this PR. Diffing the two files only reveals ember changes.

@lforst lforst merged commit 8a8e489 into master Jun 27, 2022
@lforst lforst deleted the lforst-ember-package-contents-fix branch June 27, 2022 10:16
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.

@sentry/ember package has some missing addon files
2 participants