Skip to content

feat: remove dedicated type entrypoints #4264

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

Closed
wants to merge 2 commits into from

Conversation

joshblack
Copy link
Member

@joshblack joshblack commented Feb 12, 2024

Remembered this when reviewing #4225 that we have dedicated subfolders for TypeScript types to resolve correctly and that the package.json files in these folders have a link to an issue.

It appears that this issue is closed and so I wanted to test it out to see if it worked as intended. If so, we can remove these folders in preference of the types in exports

Changelog

New

Changed

  • Add types to conditional exports in package.json

Removed

  • Remove drafts, experimental, and deprecated stub folders

Rollout strategy

  • Minor release

Testing & Reviewing

  • Verify that these folders are no longer needed in case I missed anything 😅
  • Double check that CI passes
  • For testing upstream, I opened up a PR and they seem to be okay and are failing due to unrelated changes

Copy link

changeset-bot bot commented Feb 12, 2024

🦋 Changeset detected

Latest commit: 9dba60d

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 Minor

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

Copy link
Contributor

github-actions bot commented Feb 12, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 113.37 KB (0%)
packages/react/dist/browser.umd.js 114.02 KB (0%)

@github-actions github-actions bot temporarily deployed to storybook-preview-4264 February 12, 2024 19:25 Inactive
@joshblack
Copy link
Member Author

bump @siddharthkp when you have a sec!

Copy link
Member

@siddharthkp siddharthkp left a comment

Choose a reason for hiding this comment

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

Hi!

I don't fully understand the change 😅 but trust that you have tested this with dotcom!

@joshblack
Copy link
Member Author

@siddharthkp I think the notable thing is that we had these folders in place for when TypeScript didn't fully support the "exports" config in package.json. The linked issue in the package.json files that we had has since been closed and the capabilities for this are now behind module resolution options like node16/bundler but may not be present if someone is using just node as the resolution option.

The big change in this PR is shifting types over to the export conditions using the same syntax as other entrypoints. The drawback of this is that tools like publint will warn that the types linked to in the "import" block will still be interpreted as CommonJS and not ESM since the parent package.json file has an implicit "type": "commonjs" 😬 This seems to be the main downside of dual bundling and would require us having to emit these files as .d.mts files.

Anyways, just wanted to add some more context here lol. There's a lot of moving pieces so just wanted to share before merging to get your thoughts.

@joshblack joshblack closed this Apr 10, 2024
@jonrohan jonrohan deleted the refactor/remove-type-folder-entrypoints branch May 9, 2025 18:35
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.

2 participants