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

Updating typedef to use different export syntax #18

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

Conversation

smallsaucepan
Copy link

@smallsaucepan smallsaucepan commented Jan 7, 2024

HI @rowanwins. Would you mind taking a look at this PR? Trying to update Turf to "esm first" packages, and having problems importing sweepline-intersections now that we've got node16 set as the moduleResolution mode.

It might be related to microsoft/TypeScript#50175 (comment) in that I'm getting the same initial error message when trying to import sweepline-intersections' default export - This expression is not callable ... has no call signatures

Happy to talk through it including any troubleshooting ideas you might have to try first.

…export into Turf when using node16 moduleResolution. Adding the core function as a named export as well as the default.
@rowanwins
Copy link
Owner

rowanwins commented Jan 8, 2024

Thanks for flagging @smallsaucepan - don't you love package bundling and importing issues 😂

A couple of other things come to mind for the package.json

  • adding a type key
  • adding an exports key

Making those adjustments would bring this package inline with what Turf does for it's sub-packages
https://github.com/Turfjs/turf/blob/master/packages/turf-along/package.json#L26-L42

That said I should possibly review the rollup config, because I probably don't need to include nodeResolve plugin in all the outputs.

I'm certainly no expert on package bundling, and TS inparticular can drive me a bit bonkers, but I'll take a look and see what I can work out. Can you point me at the relevant TurfJS branch where you're getting the issue?

…d.ts as per arethetypeswrong recommendation.
@smallsaucepan smallsaucepan changed the title Updating to use named exports Updating typedef to use different export syntax Jan 21, 2024
@smallsaucepan
Copy link
Author

Thanks @rowanwins. Packaging and importing is certainly a ... rich vein of potential learnings 🥲

Can you point me at the relevant TurfJS branch where you're getting the issue?

Sure thing. The branch where this popped up is https://github.com/smallsaucepan/turf/tree/type-module Take a look at turf-kinks. Pretty sure it's because we moved to node16 module resolution.

Have rolled back a couple of changes and changed the tack of the PR too. arethetypeswrong produces the below diagnostic and advice:

The resolved types use export default where the JavaScript file appears to use module.exports =. This will cause TypeScript under the node16 module mode to think an extra .default property access is required, but that will likely fail at runtime. These types should use export = instead of export default.

Would you want to add those other items (type and exports fields) in this PR? Or a separate one?

@smallsaucepan
Copy link
Author

Hmm, don't think that will work either. Can't use export = Blah if there are other exports. Will move this PR to draft and regroup.

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.

None yet

2 participants