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

feat: change to default ESM package. For developer testing and node usage in the module customization scenario. #19513

Merged
merged 3 commits into from
Jan 22, 2024

Conversation

100pah
Copy link
Member

@100pah 100pah commented Jan 15, 2024

1. Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others


2. Breaking change caused by this PR (since v5.5.0)

See #19513 (comment)



3. What does this PR do?

change to default ESM package. For developer testing and node usage in the module customization scenario.

related issues:

At present, echarts exports only esm files in npm (in lib dir of npm package).
It works well in bundler but not well in nodejs runtime and some node-based testing framework (like vitest, jest).

This PR changes:

  • Add "type": "module" to package.json
  • Add "exports": {...}" to package.json
  • Add some package.json files to the sub-dir, whose content is only "type": "commonjs".


4. Background Knowledges


4.1. How a file is recognized as ESM or CJS by nodejs runtime ?


4.2. The env (runtime and bundler) we need to handle

  • runtime (node / vitest / jest(create-react-app) / ssr / ...)
  • bundler (webpack / rollup / vite / esbuild / ...)

4.3. How a package is published as both CJS and ESM

Brief:

  • Most packages are not bothered by this issue because they only publish a bundled file. But packages that publish lots of files in lib dir probably need to consider this issue. We only discuss this scenario below.
  • Solution_A: publish two directories, one for CJS(js+dts) and another for ESM(js+dts). This solution is used more often.
  • Solution_B: use "type": "module" in the root package.json. This solution is not many appear.
  • Currently we use Solution_B for minify modifications.

References:


4.4. About "fully specified file extension"

CommonJS have file extension auto-completion but ESM require fully specified by default after "type": "module" is set to package.json.
That is,

import * as obj from 'echarts/charts';
// can not be resolved as echarts/charts.js by default in nodejs ESM.
// But we need it be resolved as echarts/charts.js for backward compat.

After "type": "module" is set to package.json, at least both nodejs and browser and some bundlers (like webpack5) follow this rule by default. It brings trouble to echarts and zrender in backward compatibility, because historically they has many internal files imported by other packages without file extension specified.

See the solution in the next section "set 'exports' in package.json".

References:


4.5. Why we need to set "exports" in package.json

"exports" field of package.json enables packages to define which files are public and which are private. Although this mechanism is useful for a package management system, but it is not introduced for a long time in npm and the spec has been changed several times, it cause that not all runtime/bundlers fully support all the features in the current spec (consider there are legecy versions of runtime/bundlers). So if we use them, we have to be very carefully. (reference: "exports" and "type": "module" is supported since nodejs v12.17.0 (and unflagged conditional exports since v13.7.0, v12.17.0) (see https://nodejs.org/dist/v16.20.2/docs/api/packages.html#modules-packages)

At present we add "exports" mainly for resolving the "file extension fully specified" issue above. (see more info about "fully specified" in the previous section.)
For example, the setting below

{
  "type": "module",
  "exports": {
    "./core": "./core.js",
    "./core.js": "./core.js"
  }
}

enables import 'echarts/core' (file extension is not specified) to work (when "type": "module" is set).

But using "exports" brings some troubles when importing lots of internal files with "file extension not specified" way.
Note that if "exports" is set, the files that are not lists in "exports" will not be visible from outside. But historically echarts and zrender has many internal files imported by other packages (and without file extension specified).
e.g. import * as echarts 'echarts/lib/echarts', import 'echarts/lib/chart/line';, ..., which are deprecated usage since v5 but widely used. And some internal utils of echarts and zrender are also been imported in that way. Currently we do not recommend that but need to keep backward compatible.

The neat way to compat it is to use the subpath patterns
e.g., make "exports" of package.json like that:

{
  "exports": {
    // ...
    "./*.js": "./*.js", 
    "./*": "./*.js"
  }
}

It routes all 'xxx' to 'xxx.js' and all 'xxx.js' to 'xxx.js' (rather than 'xxx.js.js').

But the spec of the subpath patterns seems to have been keeping changing. For example, at the beginning, it seem that only trailing wildcard is supported, that is, only supports "./xxx/*": "..." but does not support "./xxx/*.js": "...". It cause that some old versions of bundlers/runtimes do not support the newest spec. e.g., rollup plugin node-resovle v11.0.1 only supports wildcards at the end and do not support "./xxx/*.js": "...". (see impl: findExportKeyMatch and findEntrypoint ).
In this situation, if setting to { "./*.js": "./*.js", "./*": "./*.js" }, once "./*.js" is not recognized, import './xxx.js'; will be resolved as ./xxx.js.js, which is not expected.
if setting to { "./*": "./*" }, import './xxx'; will be resolved as './xxx' and can not find the file because there is no file extension.

Finally the only way I find is to list all the possible files in "exports", ending with a { "./*": "./*" }.

{
  "exports": {
    // ...
    // List the possible entries without file extension specified used outside.
    // The list is search from github.
    "./lib/chart/bar": "./lib/chart/bar.js",
    "./lib/chart/boxplot": "./lib/chart/boxplot.js",
    "./lib/chart/candlestick": "./lib/chart/candlestick.js",
    "./lib/chart/custom": "./lib/chart/custom.js",
    "./lib/chart/effectScatter": "./lib/chart/effectScatter.js",
    // ...

    // Support that `import 'echarts/xxx/xxx.js'`
    "./*": "./*"
  }
}

It's ugly. Is there any better idea?

Note: webpack

References about compat:

Note:

  • If using "exports", the files that are not declared in "exports" will be invisible from outside any more.
  • The path must start with '.'.

4.6. Use physical entry file or alias in "exports" of package.json

Although using "exports" of package.json we can make alias (or say, route) to physical file (for example: { "exports": { "./xxx": "./yyy/zzz.js" } } enables import 'echarts/xxx' to route to 'echarts/yyy/zzz.js'), at present we can not make sure all the versions of bundle tools and runtimes in use do it consistently. So we do not use the alias setting, but keep providing physical file for each public entry. For example, for an official public entry 'echarts/core', we provide a file echarts/core.js (and echarts/core.d.ts).



5. Before and After this PR


5.1. [pure nodejs]

Before this PR, echarts js like echarts/core.js can not be resolved as ESM. After this PR, they can.


5.2. [vitest]

Before this PR vitest for vue-echarts does not work.
See:

Before this PR, we can fix the issues by patch the installed 'echarts' and 'vue-echarts':

{
  "scripts": {
    "postinstall": "npx amend-package --builtin-config fix-vue-echarts-esm.config.cjs && npx amend-package --builtin-config fix-echarts-esm.config.cjs"
  }
}

After this PR, only 'vue-echarts' need to be patched for this case.

npm i amend-package

package.json

{
  "scripts": {
    "postinstall": "npx amend-package --builtin-config fix-vue-echarts-esm.config.cjs"
  }
}

After 'vue-echarts' updated in future, nothing need to be patched any more.

See more details in #19513 (comment)


5.3. [jest]


5.4. [create-react-app]

See:

create-react-app uses jest.
Event without this PR, we can also fix the issue above like that:
In package.json :

{
  "jest": {
    "transformIgnorePatterns": ["node_modules/(?!(chart\\.js|echarts|zrender)).*\\.js$"]
  }
}


6. Test

Have tested in:

  • vitest/vite
  • jest
  • react-create-app
  • webpack
  • rollup
  • esbuild
  • nodejs call directly

The test cases have been put in this repo temporarily.
https://github.com/100pah/esm-boilerplate-tmp
The test cases will be committed to echarts-examples soon if the PR accepted.



7. For legacy versions of echarts

See #19513 (comment)



8. ZRender Changes

  • This PR depends on ZRender changes (ecomfe/zrender#xxx).

ecomfe/zrender#1051



9. Note about current "exports"

Only these entries are officially exported to users:

  • 'echarts'
  • 'echarts/index.js'
  • 'echarts/index.blank.js'
  • 'echarts/index.common.js'
  • 'echarts/index.simple.js'
  • 'echarts/core.js'
  • 'echarts/charts.js'
  • 'echarts/components.js'
  • 'echarts/features.js'
  • 'echarts/renderers.js'
  • 'echarts/i18n/*
  • 'echarts/theme/*
  • 'echarts/types/*
  • 'echarts/extension/*
  • 'echarts/dist/*
  • 'echarts/ssr/client/index.js'

The other entries listed in the "exports" field of package.json make the internal files visible, but they are legacy usages, not recommended but not forbidden (for the sake of keeping backward compatible). These entries are made from the search in github about which internal files are imported.

Copy link

echarts-bot bot commented Jan 15, 2024

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

The pull request is marked to be PR: author is committer because you are a committer of this project.

⚠️ MISSING DOCUMENT INFO: Please make sure one of the document options are checked in this PR's description. Search "Document Info" in the description of this PR. This should be done either by the author or the reviewers of the PR.

Copy link
Contributor

github-actions bot commented Jan 15, 2024

The changes brought by this PR can be previewed at: https://echarts.apache.org/examples/editor?version=PR-19513@80172d6

Copy link

echarts-bot bot commented Jan 15, 2024

To reviewers: If this PR is going to be described in the changelog in the future release, please make sure this PR has one of the following labels: PR: doc ready, PR: awaiting doc, PR: doc unchanged

This message is shown because the PR description doesn't contain the document related template.

@100pah
Copy link
Member Author

100pah commented Jan 15, 2024

How to fix the issue on the previous versions of echarts (before echarts v5.5.0), vue-echarts and other packages?

The solution below works.
(But is there any risk?)

https://github.com/100pah/amend-package

npm i amend-package

In package.json :

{
  "scripts": {
    "postinstall": "npx amend-package --builtin-config fix-vue-echarts-esm.config.cjs && npx amend-package --builtin-config fix-echarts-esm.config.cjs"
  }
}

@100pah 100pah added this to the 5.5.0 milestone Jan 16, 2024
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jan 22, 2024
@Ovilia Ovilia merged commit 6b8fae8 into master Jan 22, 2024
2 checks passed
Copy link

echarts-bot bot commented Jan 22, 2024

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

@Ovilia Ovilia deleted the module_default_esm branch January 22, 2024 12:35
@100pah 100pah changed the title feat: change to default ESM package. For developer testing and node usage in customization module scenario. feat: change to default ESM package. For developer testing and node usage in the module customization scenario. Jan 22, 2024
@David-Tsui
Copy link

Love!

@100pah
Copy link
Member Author

100pah commented Jan 30, 2024

Breaking change caused by this PR (since v5.5.0)

We have tried our best to make it backward compatible, but there may be something that not considered fully.
So make the change in this major version (v5.5.0).

The possible breaking:


1. About "file extension not specified"

If using "no file extension way" to import "theme" or "i18n" or "dist" (e.g. import 'echarts/i18n/xxx') or some internal file (e.g. import xxx from 'echarts/xxx/xxx'), and the path is not listed in "exports" field of package.json, the file might not be able to find by default in some bundlers since echarts v5.5.0.

Solution_1: change to import xxx from 'echarts/xxx/xxx.js' (file extension fully specified)
Solution_2: change the config of the bundler to auto resolve file extensions. As far as I known, webpack5 do it by default and need no extra settings, but rollup not.


2. About dist/echarts.esm.js

If using import * as echarts from 'dist/echarts.esm.js' and encounter problem that can not recognize it as ESM, please use 'dist/echarts.esm.mjs' instead.


3. Import internal files in some old versions of bundle tools or NodeJS

If you import some internal file like 'echarts/lib/chart/xxx.js' or 'echarts/lib/component/xxx.js' (ended with .js), and it reports error like "Module not found. echarts/lib/chart/xxx.js is not exported ...", it might because that version of bundle tools or runtime does not support wildcard in "exports".

For example, webpack:

So if we use webpack v5.0.0~v5.2.0, the issue above might happen. Solutions:

  • Solution_A: Do not import that internal echarts files. It is not recommended since echarts v5. Use https://echarts.apache.org/handbook/en/basics/import instead.
  • Solution_B: Upgrade your bundle tool or node runtime to the latest stable version.
  • Solution_C: Use package patch tools (e.g., amend-package) to modify the installed echarts / zrender to fit your usage.

@Ovilia Ovilia added the PR: awaiting doc Document changes is required for this PR. label Feb 1, 2024
@100pah 100pah mentioned this pull request Feb 4, 2024
3 tasks
@Ovilia Ovilia added PR: doc ready and removed PR: awaiting doc Document changes is required for this PR. labels Feb 4, 2024
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.

3 participants