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(*): provide typings for both cjs and es builds #1782

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

Conversation

Septh
Copy link

@Septh Septh commented Oct 3, 2024

Rollup Plugin Name: all (except Beep)

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:
resolves #1541, #1578
supersedes #1744

Description

As soon as you start using "moduleResolution": "Node16" (or "NodeNext") in tsconfig.json, TypeScript complains that the default export for the plugins in this repo "is not callable" because it "has no call signatures".

This PR fixes this by providing each package with distinct typings for the ESM build and the CJS build. It is a follow-up of the work @danielbayley started on #1744.

@shellscape > this passes all the current tests. However, given your past comment here, I think you'd prefer some specific tests to be added... Could you please just put me on the right track for this?

@shellscape
Copy link
Collaborator

Great addition. Thank you for taking this on (and sorry for the delay in getting eyes on this. I'm in Florida and was hit by two hurricanes in the last few weeks)

You're correct. We could merge this as-is, but we'd have no protection against regressions that others might introduce. Ideally, we'd have a .ts file for each plugin that would use the types that were generated, and checked by tsc for extra assurance. Alternatively, we could snapshot the types file content in tests as a looser form of assurance. Both are lifts, so I understand if it's something you're hesitant to do. Please let us know what you think.

@Septh
Copy link
Author

Septh commented Oct 16, 2024

I went with the first option. 😄

@Septh Septh changed the title Provide typings for both cjs and es builds fix(repo): provide typings for both cjs and es builds Oct 16, 2024
@shellscape
Copy link
Collaborator

Impressive turn-around on that. A few things:

  • what's the significance of the Node16 vs Node10 directories - just support for type: module in package.json?
  • the current_package setup seems to be causing some problems on CI. I'd probably suggest just using from '../' and it should pick up the right files

@Septh
Copy link
Author

Septh commented Oct 16, 2024

  • what's the significance of the Node16 vs Node10 directories - just support for type: module in package.json?

The names refer to the moduleResolution strategy set in tsconfig.json.

I'm testing the 4 possible cases :

  • Node16 within a type: "module" directory
  • Node16 within a type: "commonjs" directory
  • Node10 (aka Node) with module: "CommonJS" in tsconfig
  • and Node10 with module: "ESNext"
  • the current_package setup seems to be causing some problems on CI. I'd probably suggest just using from '../' and it should pick up the right files

That was my first shot, but it wasn't appropriate. I did some builds with --traceResolution and it appears that TypeScript resolves '../' as a straight relative identifier rather going through the whole Node resolution algorithm. This made the tests mostly irrelevant.

I borrowed the idea of linking the package to itself to node-resolve, so if it's not working in CI maybe I did something wrong? I made symbolic links, was this the right choice?

@Septh
Copy link
Author

Septh commented Oct 16, 2024

I read on SO that symlinks have to be relative to the root of the repo (makes sense). Let's try again.

@danielbayley
Copy link

I read on SO that symlinks have to be relative to the root of the repo (makes sens). Let's try again.

@Septh FYI, there's a useful little utility for that: https://github.com/brandt/symlinks.

@Septh
Copy link
Author

Septh commented Nov 8, 2024

@shellscape > any chance you could review my latest push, please?

@shellscape
Copy link
Collaborator

@Septh taking a look now

@shellscape shellscape changed the title fix(repo): provide typings for both cjs and es builds fix(*): provide typings for both cjs and es builds Dec 15, 2024
@shellscape
Copy link
Collaborator

Looks like CI is failing on Windows. Not entirely sure why, but unfortunately I don't have the time to triage this for you, and we can't merge until that's passing.

@Septh
Copy link
Author

Septh commented Dec 15, 2024

Looks like CI is failing on Windows. Not entirely sure why, but unfortunately I don't have the time to triage this for you, and we can't merge until that's passing.

Well, I'm not sure either what's going wrong. Maybe someone reading this can provide some help? 🙏

@shellscape
Copy link
Collaborator

that's probably unlikely, unfortunately. we have a severe lack of active maintainers.

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.

Can't correctly import plugins into a TypeScript module
3 participants