Converting packages to type: module by default #2565
Conversation
…mpatibility. Flow on changes from that include requiring a js extension for relative includes, and workarounds for a couple of older third party libraries that were no longer importing smoothly. Builds successfully. Updating tests next.
…rences to __dirname. Added some re-exporting wrapper files for older packages like rbush and sweepline-intersections. See turf-line-intersects for an example.
…issues in post type: module. Maybe someone in future can exercise some typescript jujitsu to re-enable them.
smallsaucepan
added
typescript
bundling
| // Manifests as "This expression is not callable ... has no call signatures" | ||
| // https://stackoverflow.com/a/74709714 | ||
|
|
||
| import lib from "rbush"; |
| @@ -0,0 +1,7 @@ | |||
| // Get around problems with moduleResolution node16 and some older libraries. | |||
| // Manifests as "This expression is not callable ... has no call signatures" | |||
| // https://stackoverflow.com/a/74709714 | |||
There was a problem hiding this comment.
Same thing here, it appears to have default function in both the types and the code. Maybe its a weird build step from the package with the code in it?
https://github.com/rowanwins/sweepline-intersections/blob/master/src/main.js
https://github.com/rowanwins/sweepline-intersections/blob/master/types.d.ts
| // Manifests as "This expression is not callable ... has no call signatures" | ||
| // https://stackoverflow.com/a/74709714 | ||
|
|
||
| import lib from "sweepline-intersections"; |
There was a problem hiding this comment.
Ditto on sweepline-intersections
| @@ -1,5 +1,8 @@ | |||
| /* | |||
There was a problem hiding this comment.
For this package, what import was causing problems?
There was a problem hiding this comment.
Problem was cropping up with the import RBush from "rbush" line.
There was a problem hiding this comment.
With the wrapper, it looks like you're still needing to use ts-ignore and lose the rbush types in turf-clusters-dbscan/index.js?
Following the rabbit hole of one of the links you shared @smallsaucepan I found this module default-import that is also a workaround wrapper that preserves types. I haven't fully tested it in my own work (I'm doing an esm conversion) but it looks promising.
import { defaultImport } from "default-import";
import rbushDefault from "rbush";
const RBush = await defaultImport(rbushDefault);
class RBushIndex extends RBush<Feature> {
...
There was a problem hiding this comment.
Thanks for finding that Tim. It does do the trick though it looks like there may be some flow on effects. To use the const RBush = await defaultImport(rbushDefault); approach we need to change our JS target to at least es2022 to support the top level await. More so the umd package we generate for CDN usage can't have that top level await either, so it would be a change to how turf.min.js was included directly in the browser i.e. https://rollupjs.org/configuration-options/#output-format
Btw where were you seeing errors that required a ts-ignore? Previous approach seemed to work without complaining in my VSCode.
The lib/wrapper approach seems ok. We can use defaultImport, though I think that pushes us forward too quickly and I'm not sure the implications.
There was a problem hiding this comment.
No problem if can't do it. The migration I'm doing targets es2022 so I didn't think about top-level await. One idea, wonder if can wrap the dynamic import into a little IIFE like:
import { defaultImport } from "default-import";
import rbushDefault from "rbush";
const RBush = await (async () => {
const RBush = await defaultImport(rbushDefault);
return RBush
})()
class RBushIndex extends RBush<Feature> {
...
At that point you might as well keep your wrapper if it's all the same.
I guess it was a ts-expect-error that I saw, not ts-ignore. Maybe what I saw was just the original code you'd already replaced.
…ntified that the .cjs extension was causing problems for some consumers. Would have like to use .d.ts rather .d.cts for CJS type defs, but there's a but in tsup at the moment that won't allow that to be customised.
… as not supplied automatically by node any more. Updating benchmark tests. Adding some missing bench and tape @types libraries.
…removed a few that had inadvertently been added.
… Got disabled types tests working again by adjusting tsc command (needed to include node16 module and moduleResolution switched on command line). Also needed to refer to "index", etc as "index.js", etc from test.ts files to avoid "Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16'" errors.
|
Hi @mfedderly. Have done the best as I can with this for now. A summary of any item left unanswered seems to be something odd in the build or packaging of a third party library. Happy to chase these down one by one with the authors as it would be good to remove any wrapper files long term. For now though, this should be good enough for us to push ahead with the goal of developing in an ESM world, and packaging ESM+CJS. |
…ort library which takes care of finding the correct default to import from CJS packages.
…faultImport library which takes care of finding the correct default to import from CJS packages." This reverts commit f8184c3.
|
Thanks for approving @twelch. Let me clean up (roll back the defaultImport change) and then I'll merge. Appreciate your input too @mfedderly given your earlier comments. |
…module resolution.
| import { Feature, LineString } from "geojson"; | ||
|
|
||
| const __dirname = path.dirname(fileURLToPath(import.meta.url)); |
There was a problem hiding this comment.
I found a thing in my travels that tsup will polyfill these sorts of things for us.
https://tsup.egoist.dev/#inject-cjs-and-esm-shims
Given we use the ES module import syntax almost everywhere, and are exporting CJS and ESM versions anyway, we should make our modules "ESM first".
Had to disable a few type tests, though these are only in packages where we import older third party libraries.
Didn't bother replacing node __dirname references in tests as they seem to work fine within each package without.
npm testat the sub modules where changes have occurred.npm run lintto ensure code style at the turf module level.