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

Replace tsup with ts-bridge #4648

Merged
merged 7 commits into from
Sep 16, 2024
Merged

Replace tsup with ts-bridge #4648

merged 7 commits into from
Sep 16, 2024

Conversation

Mrtenz
Copy link
Member

@Mrtenz Mrtenz commented Aug 30, 2024

Explanation

ts-bridge finally supports project references. In this PR, I've swapped out tsup for ts-bridge everywhere.

References

Related to MetaMask/metamask-module-template#247, MetaMask/utils#182.
Closes #4333.

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

Copy link

socket-security bot commented Aug 30, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@ts-bridge/[email protected] None 0 314 kB mrten
npm/@ts-bridge/[email protected] filesystem, unsafe 0 130 kB mrten

🚮 Removed packages: npm/[email protected]

View full report↗︎

expectWorkspaceField(
workspace,
'exports["."].types',
'./dist/types/index.d.ts',
'exports["."].import.types',
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that types must be above default. I don't think Yarn enforces this currently, but we could look into doing so.

"files": [
"dist/"
],
"scripts": {
"build": "tsup --config ../../tsup.config.ts --tsconfig ./tsconfig.build.json --clean",
"build": "ts-bridge --project tsconfig.build.json --verbose --clean --no-references",
Copy link
Member Author

Choose a reason for hiding this comment

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

ts-bridge builds with project references if they are set in tsconfig.json by default. To build an individual package, we have to use --no-references here. I may reconsider this to make project references opt-in rather than opt-out, as that's what tsc does.

@Mrtenz Mrtenz mentioned this pull request Sep 11, 2024
3 tasks
Mrtenz added a commit that referenced this pull request Sep 12, 2024
## Explanation

This bumps all Snaps packages used in the core repo to the latest
version. This is necessary to unblock #4648.

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
@Mrtenz Mrtenz marked this pull request as ready for review September 12, 2024 14:35
@Mrtenz Mrtenz requested review from a team as code owners September 12, 2024 14:35
package.json Outdated Show resolved Hide resolved
yarn.config.cjs Outdated Show resolved Hide resolved
@@ -488,20 +490,35 @@ async function expectWorkspaceLicense(workspace) {
function expectCorrectWorkspaceExports(workspace) {
Copy link
Contributor

@MajorLift MajorLift Sep 13, 2024

Choose a reason for hiding this comment

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

Would there be a way to add the same rules for something like 'exports.*.import.types'? Some core packages have additional subpaths in the exports field, and we may need to enforce that those expose dual CJS/ESM builds as well.

e.g. https://github.com/MetaMask/core/blob/main/packages/notification-services-controller/package.json

Copy link
Member Author

Choose a reason for hiding this comment

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

I made the minimal modifications to the constraints to have a valid main entry point for now. This would certainly be possible, but I feel like it's out of scope for this PR. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed! We can take that on in a separate ticket along with the necessary fixes to those exports subfields.

Copy link
Contributor

Choose a reason for hiding this comment

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

Created a ticket here: #4699

Copy link
Contributor

@MajorLift MajorLift left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your work on this, including all the amazing work leading up to this on ts-bridge.

Copy link
Contributor

@AugmentedMode AugmentedMode left a comment

Choose a reason for hiding this comment

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

LGTM

@Mrtenz Mrtenz merged commit 1d12f7e into main Sep 16, 2024
116 checks passed
@Mrtenz Mrtenz deleted the mrtenz/ts-bridge branch September 16, 2024 09:00
@Mrtenz Mrtenz mentioned this pull request Sep 16, 2024
3 tasks
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.

tsup's code splitting feature makes core packages difficult to debug
5 participants