Types not being exported in a robust way

[smallsaucepan]

Hi @luizbarboza. We're quite keen to use polyclip-ts over in Turfjs. However having some issues gettings the types imported e.g.

index.ts(10,27): error TS7016: Could not find a declaration file for module 'polyclip-ts'. '/Users/james/Code/turf/cmurphy23/node_modules/.pnpm/polyclip-ts@0.16.6/node_modules/polyclip-ts/dist/index.js' implicitly has an 'any' type.
  There are types at '/Users/james/Code/turf/cmurphy23/packages/turf-intersect/node_modules/polyclip-ts/types/index.d.ts', but this result could not be resolved when respecting package.json "exports". The 'polyclip-ts' library may need to update its package.json or typings.

One fix would be to flesh out the exports section of polyclip-ts/package.json, so would be happy to prepare a PR for that. However, without packaging CJS version of the library as well, the best we could hope for from arethetypeswrong would be:

Would you like a PR that will generate and export CJS also? Or do you prefer to stick with ESM only?

Thanks for maintaining the package!

[luizbarboza]

Hi! Thanks for the suggestion and for supporting the package!

If possible, could you split it into two PRs—one to fix the type export issue and another to add CJS support? This would make the review process easier.

[smallsaucepan]

Sure thing 👍 Will do the smaller type export one first.

[smallsaucepan]

Oh, actually just saw #8, which had a similar goal to this issue. That PR was merged in e079fa8 but then rolled back some months later by 7bfaa6c.

Did something go wrong? Couldn't find any chatter about the reason.

[luizbarboza]

The PR was merged but rolled back before publishing a new version on npm. It references a PR in another repository that hasn’t been merged. I still have doubts about the ideal solution and want to be cautious with breaking changes.

[smallsaucepan]

Ah, understand. What I have in mind is quite similar to @vschroeter's PR, and a lot of the discussions they're having on the vite PR Turf has had too.

Appreciate you being cautious, and I certainly don't want to ruin anyone's day. To test we can implement a bunch of dummy clients and make sure we don't introduce any errors. Do you get any stats on who is using the library? Node or browser versions?

One thing I'd like to know more about is the existing "umd": "..." entry in exports. Not seen that before, and can't find it documented anywhere. If I understand this documentation it would act as a subpath should someone try to import * as whatever from "polyclip-ts/umd"? Do you recall which client/s that was intended for?

[smallsaucepan]

Hi @luizbarboza. Do you have any thoughts on the above?

[luizbarboza]

@smallsaucepan Honestly, I’m not entirely sure what is causing the error when resolving types.

The UMD bundle is intended for legacy environments and can be used via a CDN, as explained in the README.

A previous PR that was reverted had removed the UMD entry, but it’s important to keep it for backward compatibility. Could you submit a PR to fix the issue while preserving the UMD entry?

[smallsaucepan]

Honestly, I’m not entirely sure what is causing the error when resolving types.

@luizbarboza it's a couple of things. The immediate problem solved below is that typescript tools will (without further config) look for typedefs in the same directory as dist/index.js. But the index.d.ts is currently in the types/ directory. That's why we get errors in rows 3. and 4. below.

┌───────────────────┬──────────────────────────────┐
│                   │ "polyclip-ts"                │
├───────────────────┼──────────────────────────────┤
│ node10            │ 🟢                           │
├───────────────────┼──────────────────────────────┤
│ node16 (from CJS) │ ❌ No types                  │
│                   │ ⚠️ ESM (dynamic import only) │
├───────────────────┼──────────────────────────────┤
│ node16 (from ESM) │ ❌ No types                  │
├───────────────────┼──────────────────────────────┤
│ bundler           │ ❌ No types                  │
└───────────────────┴──────────────────────────────┘

I have a four line PR that leaves umd untouched and gets you to this:

┌───────────────────┬──────────────────────────────┐
│                   │ "polyclip-ts"                │
├───────────────────┼──────────────────────────────┤
│ node10            │ 🟢                           │
├───────────────────┼──────────────────────────────┤
│ node16 (from CJS) │ ❌ No types                  │
│                   │ ⚠️ ESM (dynamic import only) │
├───────────────────┼──────────────────────────────┤
│ node16 (from ESM) │ 🟢 (ESM)                     │
├───────────────────┼──────────────────────────────┤
│ bundler           │ 🟢                           │
└───────────────────┴──────────────────────────────┘

No support for CJS require, same as current library. It can be added though that's a bit more involved. Happy to go with your preference.

Actually just remembered your comment above about adding CJS support separately. Will create a PR just for fixing up ESM types 👍

[smallsaucepan]

Thanks for merging that @luizbarboza. Are you planning to release 0.16.7 with just this PR, or do you want to do the CJS generation as well first?

[luizbarboza]

Version 0.16.7 will be released with just this PR, leaving support for CJS for a later new version.

[smallsaucepan]

Wonderful. Thank you!