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(repo): type definitions not being exported correctly for esm modules #1744

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

danielbayley
Copy link

@danielbayley danielbayley commented Jun 30, 2024

Rollup Plugin Name: {name}

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

Picking up the baton from @Geylnu in #1578

I also ran into this. As mentioned elsewhere, the types are wrong in all Rollup plugins! Meaning they do not play nice with TypeScript.

More detailed explanations can be found here and here.

Fixes #1541.
Fixes #1329.

@danielbayley danielbayley changed the title Types fix(repo): type definitions not being exported correctly for esm modules Jun 30, 2024
@shellscape
Copy link
Collaborator

Since this affects all plugins in the repo, this PR also needs to assert that every plugin has a types test. Moving this away from sibling types is opening an entirely new can of worms, and types now need to be verified for both commonjs and ES tsconfig#module users.

Had the last PR made it beyond CI, that would have been the ask as well.

@danielbayley
Copy link
Author

Since this affects all plugins in the repo, this PR also needs to assert that every plugin has a types test. Moving this away from sibling types is opening an entirely new can of worms, and types now need to be verified for both commonjs and ES tsconfig#module users.

Had the last PR made it beyond CI, that would have been the ask as well.

@shellscape Sure, will circle back to this and look into it…

@shellscape
Copy link
Collaborator

@danielbayley last call on this one

@Septh
Copy link

Septh commented Sep 23, 2024

@danielbayley last call on this one

This PR makes Node choke when importing certain plugins (typescript and babel, at least) from an ESM package. Generating minimal package.json files next to the es bundles shouldn't be disabled.

image

Nevertheless, the types issue is a very, very long-standing one and needs to be fixed. I'm willing to take over if @danielbayley cannot pursue himself.

Since this affects all plugins in the repo, this PR also needs to assert that every plugin has a types test. Moving this away from sibling types is opening an entirely new can of worms, and types now need to be verified for both commonjs and ES tsconfig#module users.

@shellscape > none of the packages currently have types tests; what would you like to see, exactly?

@danielbayley
Copy link
Author

@danielbayley last call on this one

@shellscape I've been on holiday, then had COVID, and a bunch of other life-in-the-way stuff… So only just getting back into things. This is certainly still on my list to fix!

Nevertheless, the types issue is a very, very long-standing one and needs to be fixed. I'm willing to take over if @danielbayley cannot pursue himself.

Yeah it needs to be resolved! @Septh You're welcome to lend a hand with this PR…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants