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

build: pure ESM #106

Merged
merged 5 commits into from
Oct 9, 2024
Merged

build: pure ESM #106

merged 5 commits into from
Oct 9, 2024

Conversation

jjangga0214
Copy link
Contributor

@jjangga0214 jjangga0214 commented Oct 9, 2024

Pure ESM

Check this out.

Node version

You should deliberately set the minimum supported node version of the public library.

In the case of @livekit/agents, I felt an impression that @types/node=22 was specified without a clear intention of support. Using @types/node=22 means, you only check types against 22, so you may miss chances that older versions of nodejs do not support some APIs. Conventionally, it's likely that a new runtime introduces a few new built-in APIs, but dropping out of old APIs is much less likely, though it happens.

It's always OK that you set the latest runtime version as the official minimum guarantee, no matter whether it actually works on older runtimes. But I doubted you set the version number by conventional criteria.

I set the node version from the root level. Of course, you can override it package-wise as well.
I chose 18, as it's a rational starting point to fully support ESM.
Testing and changing this is up to you maintainer, but remember, you should set it by clear intention.

Copy link

changeset-bot bot commented Oct 9, 2024

⚠️ No Changeset found

Latest commit: b1ccc73

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@CLAassistant
Copy link

CLAassistant commented Oct 9, 2024

CLA assistant check
All committers have signed the CLA.

@nbsp
Copy link
Member

nbsp commented Oct 9, 2024

we target node 22, though we make an attempt to support version 20 within reason, and so far there hasn't been any reason not to. node 18 is in maintenance LTS, and will end-of-life in only a couple of months.

as for the node: prefix, i personally don't see much of a reason to change everything to it? i value consistency over "correctness", and regardless, the node documentation specifies it is "supported as an alternative means" and not the new correct way to do things.

@jjangga0214
Copy link
Contributor Author

jjangga0214 commented Oct 9, 2024

we target node 22, though we make an attempt to support version 20 within reason, and so far there hasn't been any reason not to. node 18 is in maintenance LTS, and will end-of-life in only a couple of months.

That's completely OK. Then you should make it clear. Placing @types/node only on @livekit/agents is not a good practice. What about other packages? I suggest specifying the default node version from the root and overriding it package-wisely if needed.

as for the node: prefix, i personally don't see much of a reason to change everything to it? i value consistency over "correctness", and regardless, the node documentation specifies it is "supported as an alternative means" and not the new correct way to do things.

  • I never said that it's a matter of correctness.
  • It's a new recommended way, but optional.
  • This is helpful to avoid naming conflict. Once the community adopted it wide enough, the Nodejs team becomes able to introduce a new built-in module without finding a fresh name that does not exist on npm.
  • It's also URL scheme compatible. This is what other languages' module systems do as well.

Would you make a final decision on whether to adopt it as a new practice?

@nbsp
Copy link
Member

nbsp commented Oct 9, 2024

those are fair points. would you mind cleaning up this PR to just have the node: import changes, so i can merge it? i'll tackle the node version issue in a separate one (we squash PRs and i'd rather this be two distinct commits)

@jjangga0214
Copy link
Contributor Author

OK, removed them.

@nbsp nbsp merged commit 221d6f4 into livekit:main Oct 9, 2024
4 checks passed
@nbsp
Copy link
Member

nbsp commented Oct 9, 2024

thanks!

@jjangga0214 jjangga0214 deleted the build/pureesm branch October 9, 2024 10:44
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.

3 participants