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

Dual Package ESM + CJS WIP #57

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

irfan798
Copy link
Contributor

@irfan798 irfan798 commented Dec 13, 2024

Why

While developing our own package we came across a problem with CBOR2 only being ESM. Normally that is great and we would like to switch ESM only but our package is consumed by legacy code as well and we cannot force them to switch.

And even if cbor2 is used as a dependency of a dependency it breaks commonjs consumers.

And most of react-native projects are using commonjs by default unfortunately.

What has changed:

Package.json

Updated exports to reflect entrypoints

Tsconfig:

Module is set to node16 because :
https://www.typescriptlang.org/docs/handbook/modules/reference.html#the-module-compiler-option

A common misconception is that node16 and nodenext only emit ES modules. In reality, node16 and nodenext describe versions of Node.js that support ES modules, not just projects that use ES modules. Both ESM and CommonJS emit are supported, based on the detected module format of each file. Because node16 and nodenext are the only module options that reflect the complexities of Node.js’s dual module system, they are the only correct module options for all apps and libraries that are intended to run in Node.js v12 or later, whether they use ES modules or not.

Module format detection

  • .mts/.mjs/.d.mts files are always ES modules.
  • .cts/.cjs/.d.cts files are always CommonJS modules.
  • .ts/.tsx/.js/.jsx/.d.ts files are ES modules if the nearest ancestor package.json file contains "type": "module", otherwise CommonJS modules.

Important keyword is nearest ancestor package.json here. If we set up file structure like this:

lib
├── esm
│   └── index.js
├── cjs
│   └── index.js

Since file names end with .js Node js will checks top level package.json and determines all files are esm because of type:module in package.json

Tsup config

  • entry: same result can be achieved by regex
  • sourcemap: makes debugging a lot easier
  • defineConfig: I separated into 2 halves, it lets us control indivudual settings like output folder and plugins

Lib folder structure

Current structure is:

lib
├── cjs
│   └── index.cjs
└── index.js (esm)

I selected this so that all cjs are aggregated, and can be deleted if not used.

Same folder

lib
├── index.cjs (commonjs)
└── index.js (esm)

This can be configured easily if you want to.

Seperate folder js extension

lib
├── esm
│   └── index.js
│   └── package.json
├── cjs
│   └── index.js
│   └── package.json

This required separate packag.json and just type set to either module and commonjs.
But tsup have a little problem with file extensions.

Seperate folder .mjs | cjs extension

lib
├── esm
│   └── index.mjs
├── cjs
│   └── index.cjs

We need to update tests and all imports for this

Tsup patch

Taken from: egoist/tsup#953 (comment)

Tsup can update file extensions when compiling but our imports are using .js extension as per required.
Tsup does not updates contents of files to reflect its .cjs changes

If we decide to use .js on both commonjs and ecmascript it solves the first problem but then if we tsup settings to change extension, even though is has dts settings, type generators does not respect this.
https://tsup.egoist.dev/#output-extension

export default defineConfig({
  outExtension({ format }) {
    return {
      js: `.${format}.js`,
      dts: `.d.ts`, // This is not respected
    }
  },
})

Types safety

Types script emulates how Nodejs>12 works when module is set to node16 or nodenext

But types for each csj and mjs and package.json exports need to be properly set up.
We can use this tool for this:
npx --yes @arethetypeswrong/cli --pack .

image

Node 10 is not supported because it doesnt know how to read package.json exports.
But we dont need to support it because of:

  "engines": {
    "node": ">=18"
  }

BUT if consumer package is using typescript and --moduleResolution node (node10) their typescript will not be able to consume our package even if they are using node>10

https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/NoResolution.md#true-positive-node-10-doesnt-support-packagejson-exports

Dual Package Hazard

https://nodejs.org/docs/latest-v18.x/api/packages.html#dual-package-hazard

In this package since Tag class has a state, it is possible that one package will import cjs version and add a new tag while esm version will try to use that added tag and will not be able to find it.

I am not use how to proceed here...

Referencing here: https://gustawdaniel.com/posts/en/simplest-tutorial-for-esm-commonjs-package-creators/
image

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (ef0e9f7) to head (2db7b11).

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #57   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        17           
  Lines         4045      4045           
  Branches       585       585           
=========================================
  Hits          4045      4045           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hildjj
Copy link
Owner

hildjj commented Dec 13, 2024

I really don't want to publish CJS with an EJS wrapper.

@irfan798
Copy link
Contributor Author

I really don't want to publish CJS with an EJS wrapper.

Me too, I am looking into options

@hildjj
Copy link
Owner

hildjj commented Dec 13, 2024

One requirement that is not negotiable: you MUST be able to use this in a browser environment without modification or tooling.

I'm ok with requiring tooling for CJS users.

Also, "just" upgrade your node version to the latest, and require('cbor2') works, since there is nothing async on the load path.

@irfan798
Copy link
Contributor Author

I am not the final consumer, our package is going to consumed by react-native and commonjs projects as well.
My priority is ESM, but also supporting commonjs consumers.

I am testing if it would be possible to write a wrapper on our end without exposing csj on your side to not have state problems.

@hildjj
Copy link
Owner

hildjj commented Dec 13, 2024

Anyone who really needs cjs support and react-native shims can go back to node-cbor, I guess. They would have to live with a lack of updates, but they're certainly used to that if they've chosen a hobbled environment.

@hildjj
Copy link
Owner

hildjj commented Dec 13, 2024

Tag is also not the only dual-package hazard. There are several places that do instanceof checks, which will fail with objects from the parallel package.

@irfan798 irfan798 marked this pull request as draft December 19, 2024 15:06
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