Add type: module to package.json #413

zaporter-work · 2024-11-12T17:54:05Z

The teleop team is trying to switch our usage of TabularDataByMQL to use the ts-sdk wrapper (so that we can use a stable function but allow the data team to change the api as-desired)

Ran into:

ui start: (node:95053) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
ui start: (Use `node --trace-warnings ...` to show where the warning was created)
ui start: 12:53:00 PM [vite] Error when evaluating SSR module /src/lib/api/client.ts: failed to import "@viamrobotics/sdk"
ui start: |- /home/zack/work/app/node_modules/.pnpm/@[email protected]/node_modules/@viamrobotics/sdk/dist/main.es.js:22354
ui start: export {
ui start: ^^^^^^
ui start: SyntaxError: Unexpected token 'export'

package.json

zaporter-work · 2024-11-12T18:23:06Z

package.json

-    "prebuild": "ts-node ./scripts/write-versions.ts",
+    "prebuild": "tsx ./scripts/write-versions.ts",


ts-node struggles to run when package.json has type: module. Ran into this: TypeStrong/ts-node#1062 (The maintainers say to swap to cjs and get 600 👎 reactions)

Hit my head against this for ~20 minutes. I might be missing something small... I didn't want to mess with the ts-config for the entire project just for this helper script. And I can't figure out what cli args would fix this.

Decided it was easier to swap to tsx https://www.npmjs.com/package/tsx . I've used this for personal projects and have had no troubles with this.

If you look at that thread, all of the mentioned prs are of the form swap to tsx from ts-node in order to get around this issue.

Opinions?

Ya probably better to use tsx? Seems like everything I can find online about it is positive

scripts/write-versions.ts is tiny and has no type annotations. IMO we should just rename it scripts/write-versions.js

@njooma Do you expect to have other more complex scripts that would benefit from types? If so, might be worth keeping tsx. If not, agreed that typescript isn't providing much value.

Let's strip it for now and if we add more complex scripts then we can deal with it then. no need to keep it around for a "maybe" if it's entirely unused now (aside from this use case)

Swapped to .js in 2fd1e38

package.json

njooma · 2024-11-12T19:48:50Z

package.json

-    "prebuild": "ts-node ./scripts/write-versions.ts",
+    "prebuild": "tsx ./scripts/write-versions.ts",


Ya probably better to use tsx? Seems like everything I can find online about it is positive

mcous · 2024-11-13T14:55:47Z

package.json

@@ -5,6 +5,7 @@
  "main": "./dist/main.umd.js",
  "module": "./dist/main.es.js",
  "types": "./dist/main.d.ts",
+  "type": "module",


Gave this some more thought: with the switch to type: module, I think we'll want to reorder exports while we're here, too. Since our default is now ESM, we'll want to put the require condition above the import condition, to ensure conditions are most-to-least specific. We may also want to rename the import condition to default.

We could also consider dropping the CJS export and UMD build to make things easier on ourselves. Dual format packages are dicey at the best of times, and I don't know if there's a really compelling need to publish CJS (unless there's something specific like react-native support that requires it)

This has been a fun deep-dive to read into. I had no idea exports were sorted. Thanks for thinking about this!

we'll want to put the require condition above the import condition, to ensure conditions are most-to-least specific.

Reading https://nodejs.org/api/packages.html#conditional-exports , it seems like the order of import and require is not important because they are entirely mutually exclusive. Am I missing a case where it would be ambiguous which export to use? Though, I like the idea of switching to require -> default (seems like a good change)

This dual-packaging example from nodejs matches our use case: https://github.com/nodejs/package-examples?tab=readme-ov-file#approach-2-isolate-state

They have the following package.json example:

{ "type": "module", "exports": { "import": "./index.mjs", "require": "./index.cjs" } }

We could also consider dropping the CJS export and UMD build to make things easier on ourselves

I like the idea. The dual-packaging docs mention that the global state of the cjs module is not shared with the es module -- which would interact poorly with the ts-sdks extensive use of globals. (though, admittedly, I think it is unlikely that people are exporting cjs libraries using the ts-sdk)

--

Deferring to the sdk team because they know more about consumption of this package.

How would dropping CJS and UMD support affect the usable environments of the SDK? I want to make sure that the SDK is available for users in the most common environments. Is there any env you can think of where a user has to use CJS or UMD?

I'm not sure how this affects react-native/node support. That should be tested

I've spent the last 20 minutes researching cjs vs esm in 2024 and I can't find any solid evidence that cjs is actually deprecated / dying (despite my strong bias towards esm). Multiple github issues threads on the nodejs forms indicate strongly that cjs isn't going anywhere and is still a first-class packaging format.

So to revisit the original comment:

We could also consider dropping the CJS export and UMD build to make things easier on ourselves

Are we actually running into difficulty with the current CJS & UMD setup? Is it worth dropping and potentially having to re-add if some customer needs to use legacy code?

I suspect the answer to both of these is a slight no. Happy to be wrong if people feel strongly.

I have swapped import -> require to require -> default in 2fe46c8 . I have imported this into the app and verified that everything works.

At this point, if it is okay with everyone involved, I would like to move forward with this PR mostly as-is (unless there are potentially backwards-incompatible issues).

Agreed, I don't think any difficulty we currently face is worth dropping CJS for now.

njooma · 2024-11-14T18:50:58Z

package.json

@@ -5,6 +5,7 @@
  "main": "./dist/main.umd.js",
  "module": "./dist/main.es.js",
  "types": "./dist/main.d.ts",
+  "type": "module",


Agreed, I don't think any difficulty we currently face is worth dropping CJS for now.

add type: module

435eac9

zaporter-work requested a review from a team as a code owner November 12, 2024 17:54

zaporter-work requested review from njooma and purplenicole730 November 12, 2024 17:54

zaporter-work commented Nov 12, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

zaporter-work requested a review from mcous November 12, 2024 17:55

Swap to tsx

26f9a49

zaporter-work commented Nov 12, 2024

View reviewed changes

njooma requested changes Nov 12, 2024

View reviewed changes

revert bump

ad6e215

zaporter-work requested a review from njooma November 12, 2024 20:04

zaporter-work added 2 commits November 12, 2024 15:09

remove types

2fd1e38

style: add back file suffix

e78687e

mcous approved these changes Nov 13, 2024

View reviewed changes

swap to default + require

2fe46c8

njooma approved these changes Nov 14, 2024

View reviewed changes

zaporter-work merged commit ed26414 into viamrobotics:main Nov 15, 2024
3 checks passed

zaporter-work deleted the zp/type-module branch November 15, 2024 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type: module to package.json #413

Add type: module to package.json #413

zaporter-work commented Nov 12, 2024 •

edited

Loading

zaporter-work Nov 12, 2024 •

edited

Loading

njooma Nov 12, 2024

mcous Nov 12, 2024

zaporter-work Nov 12, 2024

njooma Nov 12, 2024

zaporter-work Nov 12, 2024

njooma Nov 12, 2024

mcous Nov 13, 2024

zaporter-work Nov 13, 2024

njooma Nov 13, 2024

zaporter-work Nov 13, 2024

njooma Nov 14, 2024

njooma Nov 14, 2024

		"prebuild": "ts-node ./scripts/write-versions.ts",
		"prebuild": "tsx ./scripts/write-versions.ts",

Add type: module to package.json #413

Add type: module to package.json #413

Conversation

zaporter-work commented Nov 12, 2024 • edited Loading

zaporter-work Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaporter-work commented Nov 12, 2024 •

edited

Loading

zaporter-work Nov 12, 2024 •

edited

Loading