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 exports for ESM NodeNext TypeScript support #29

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

benasher44
Copy link
Contributor

@benasher44 benasher44 commented Jan 5, 2024

Closes #28

After this, here is the result from attw:

image

I also confirmed that this fixes the local issue I was having.

To fix CJS, you would need a copy of all of the index.d.ts as index.d.cts, which I was not able to to figure out how to do easily with rollup. That said, this PR is a net improvement, and I would argue great support for NodeNext + ESM is better than worrying too much about the NodeNext + CJS case (cjs without nodenext is still fine).

This also switches from bundlesize to bundlemon. I could not npm i due to issues with bundlesize, so I switched to bundlemon, which isn't abandoned.

@benasher44 benasher44 changed the title Improve ESM TypeScript support Improve ESM NodeNext TypeScript support Jan 5, 2024
@CLAassistant
Copy link

CLAassistant commented Jan 5, 2024

CLA assistant check
All committers have signed the CLA.

@nikku
Copy link
Member

nikku commented Jan 9, 2024

Thanks for your contribution. Could you please split appart the changes you're doing:

  • using bundlemon: Seems needed
  • fixing the type path(s) with extension (safe to merge in any case)
  • exposing exports: we're currently internally discussing our approach

@benasher44
Copy link
Contributor Author

Yep sure! I'll work on that.

@benasher44 benasher44 changed the title Improve ESM NodeNext TypeScript support Fix exports for ESM NodeNext TypeScript support Jan 9, 2024
@benasher44
Copy link
Contributor Author

Okay changes are now split between this one, #30, and #31.

@benasher44
Copy link
Contributor Author

Hi @nikku! I wanted to check in to see if you and your team had a chance to discuss?

@nikku
Copy link
Member

nikku commented Jan 17, 2024

@benasher44 Thanks for the poke. We're still in discussion what to do with these type definitions, as they will break CJS support.

Currently our favorite is to publish an ESM only version of this project moving forward.

@benasher44
Copy link
Contributor Author

@nikku CJS support should work fine. It's only broken for CJS when the module system is set to NodeNext/Node16, but that was broken already. Maybe you're seeing something I'm not though? Happy to fix

@benasher44
Copy link
Contributor Author

I'm also supportive of going ESM-only! I don't think that's necessary though, if you all aren't ready for that kind of breaking change.

@benasher44
Copy link
Contributor Author

Could we at minimum merge #31? That'll fix the local compile errors I'm seeing. This one will further improve the situation, but it's fine to table.

@nikku
Copy link
Member

nikku commented Jan 17, 2024

Updated this PR with a simplified version of the export, based on main. Verified it works for CJS and ESM use-cases.

@nikku nikku merged commit 43d671f into bpmn-io:main Jan 17, 2024
4 checks passed
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.

Support TypeScript NodeNext module resolution
3 participants