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.

Sign up for GitHub

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

Jump to bottom

Add type: module to package.json #413

Merged
merged 6 commits into from
Nov 15, 2024
Merged

Add type: module to package.json #413

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
435eac9
add type: module
zaporter-work Nov 12, 2024
26f9a49
Swap to tsx
zaporter-work Nov 12, 2024
ad6e215
revert bump
zaporter-work Nov 12, 2024
2fd1e38
remove types
zaporter-work Nov 12, 2024
e78687e
style: add back file suffix
zaporter-work Nov 12, 2024
2fe46c8
swap to default + require
zaporter-work Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
{
"name": "@viamrobotics/sdk",
"version": "0.29.0",
"version": "0.29.1",
zaporter-work marked this conversation as resolved.
Show resolved Hide resolved
"description": "",
"main": "./dist/main.umd.js",
"module": "./dist/main.es.js",
"types": "./dist/main.d.ts",
"type": "module",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"files": [
"dist"
],
Expand Down
Loading