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

Switch to ESM #348

Merged
merged 2 commits into from
Dec 29, 2023
Merged

Switch to ESM #348

merged 2 commits into from
Dec 29, 2023

Conversation

jonkoops
Copy link
Collaborator

Closes #238

@jonkoops
Copy link
Collaborator Author

Before merging this in we should agree on a strategy in #346.

@dcousens
Copy link
Collaborator

dcousens commented Dec 29, 2023

@jonkoops if we're compiling anyway, why not TypeScript native? Seeing as we're only 1 file deep for each of the exports, you won't need a bundler either

@dcousens
Copy link
Collaborator

dcousens commented Dec 29, 2023

I made an example project in https://github.com/dcousens/monorepo-typescript recently which I'm often using as a reference for this kind of project lift, albeit for monorepos

@jonkoops
Copy link
Collaborator Author

if we're compiling anyway, why not TypeScript native? Seeing as we're only 1 file deep for each of the exports, you won't need a bundler either

You are referring to Rollup in the benchmarks directory? I am actually looking to eliminate that entirely after this PR. In theory, all we need is a static file server to be able to import ESM code, which hopefully also eliminates any strange compiler quirks/overhead in the benchmarks.

I think a bundler/compiler (e.g Rollup/TypeScript would make sense if we decide to also support CommonJS next to ESM, but I am not sure if we would want to do so.

Perhaps we merge this in now, and make this a separate point of discussion/log an isssue? WDYT?

@dcousens
Copy link
Collaborator

dcousens commented Dec 29, 2023

I am not sure if we would want to do so.

We might see slower uptake of 3.0.0 if we opt for ESM only - this consideration is the only thing holding back a merge for me, assuming we don't want to block the next release.

Perhaps we merge this in now, and make this a separate point of discussion/log an isssue? WDYT?

Happy, this is 90% of the work, changing to TS and bundling can easily be a separate discussion, but it needs to be resolved if we want to support CJS.

bind.js Outdated Show resolved Hide resolved
bind.js Outdated Show resolved Hide resolved
bind.js Outdated Show resolved Hide resolved
bind.js Outdated Show resolved Hide resolved
bind.js Outdated Show resolved Hide resolved
dedupe.js Outdated Show resolved Hide resolved
dedupe.js Outdated Show resolved Hide resolved
dedupe.js Outdated Show resolved Hide resolved
dedupe.js Outdated Show resolved Hide resolved
dedupe.js Outdated Show resolved Hide resolved
dedupe.js Outdated Show resolved Hide resolved
dedupe.js Outdated Show resolved Hide resolved
dedupe.js Outdated Show resolved Hide resolved
dedupe.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
dedupe.js Outdated Show resolved Hide resolved
dedupe.js Outdated Show resolved Hide resolved
dedupe.js Outdated Show resolved Hide resolved
@dcousens dcousens changed the title Switch fully to ES Modules Switch to ESM Dec 29, 2023
dedupe.js Outdated Show resolved Hide resolved
@remcohaszing
Copy link
Contributor

I think a bundler/compiler (e.g Rollup/TypeScript would make sense if we decide to also support CommonJS next to ESM, but I am not sure if we would want to do so.

Please don't do this. Although many people do so, TypeScript isn't intended to be used for dual publishing. Especially the combination of TypeScript, default exports, and Rollup is a well-known recipe for generating invalid type definitions.

@jonkoops
Copy link
Collaborator Author

jonkoops commented Dec 29, 2023

Let's continue the CommonJS/TypeScript discussion under #349, and focus on landing this work here.

@jonkoops
Copy link
Collaborator Author

@dcousens WDYT? Can we merge this in?

@dcousens dcousens merged commit 413df74 into main Dec 29, 2023
5 checks passed
@dcousens dcousens deleted the esm branch December 29, 2023 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert to ESM only
3 participants