react icon indicating copy to clipboard operation
react copied to clipboard

feat: remove dedicated type entrypoints

Open joshblack opened this issue 1 year ago • 2 comments

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

  • [x] 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

joshblack avatar Feb 12 '24 19:02 joshblack

🦋 Changeset detected

Latest commit: 9dba60ddca5abef92304db6fe0aa5946d05fbffe

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

changeset-bot[bot] avatar Feb 12 '24 19:02 changeset-bot[bot]

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[bot] avatar Feb 12 '24 19:02 github-actions[bot]

bump @siddharthkp when you have a sec!

joshblack avatar Feb 26 '24 17:02 joshblack

@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 avatar Feb 28 '24 16:02 joshblack