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(bundle): remove inline @github deps #2404

Merged
merged 4 commits into from
Oct 7, 2022
Merged

Conversation

joshblack
Copy link
Member

This PR updates the rollup config to not inline @github dependencies in CommonJS. This is due to two problems when packaging:

  • The node_modules folder when lib is ignored when publishing
  • The require() statement within _FormattingTools.tsx is not being rewritten to the inlined dependency when enabled

To unblock 35.11, this PR reverts this change back to the existing behavior until we can resolve the two points above.

@joshblack joshblack requested review from a team and mperrotti October 4, 2022 17:58
@changeset-bot
Copy link

changeset-bot bot commented Oct 4, 2022

🦋 Changeset detected

Latest commit: 9898d4e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2022

size-limit report 📦

Path Size
dist/browser.esm.js 77.19 KB (0%)
dist/browser.umd.js 77.81 KB (0%)

@joshblack joshblack temporarily deployed to github-pages October 4, 2022 18:05 Inactive
@joshblack joshblack mentioned this pull request Oct 4, 2022
external.filter(id => {
return !id.startsWith('@github')
})
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious why this filter was added in the first place. Will removing it cause any side effects?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @pksjce! 👋

The background for the filter was to inline the ESM-only dependencies from @github in the CommonJS bundle to get that working again. This was kind of a trade-off until we moved to ESM-only and don't have to support CommonJS anymore

Copy link
Collaborator

Choose a reason for hiding this comment

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

As long as we are fine with ESM only, LGTM!

@joshblack
Copy link
Member Author

joshblack commented Oct 5, 2022

Following up on this, it seems like it may be due to the workflow publishing using yarn which is not including files in files 🤔

The canary publishes that use changeset work as-intended so the update here may need to be to update the RC workflow to mirror the canary one. Followed up over in Slack to see if we can fix it.

@broccolinisoup
Copy link
Member

This looks good to me!

@joshblack joshblack temporarily deployed to github-pages October 7, 2022 15:10 Inactive
@joshblack joshblack merged commit 1d1d07b into main Oct 7, 2022
@joshblack joshblack deleted the fix/add-github-to-external branch October 7, 2022 15:18
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