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

add .js extension in the import statement #16276

Merged
merged 4 commits into from
Dec 24, 2021
Merged

add .js extension in the import statement #16276

merged 4 commits into from
Dec 24, 2021

Conversation

pissang
Copy link
Contributor

@pissang pissang commented Dec 24, 2021

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

As discussed in #16054. This PR trys to add an extra transform step to add .js extension to the import statement after lib is generated.

So before the statement is:

import * as zrender from 'zrender/lib/zrender';

Now it becomes:

import * as zrender from 'zrender/lib/zrender.js';

So specific modules in the lib can be imported in the browsers more conveniently.

Misc

Related test cases or examples to use the new APIs

Still there some required steps needs to be done before we actually importing the modules:

  1. For the modules from dependencies. Import map need to be configured.
<script type="importmap">
{
  "imports": {
    "echarts/": "https://unpkg.com/echarts@latest/",
    "zrender/": "https://unpkg.com/zrender@latest/",
    "tslib": "https://unpkg.com/tslib@2.3.1/tslib.es6.js"
  }
}
</script>
  1. Because the generated code use process.env.NODE_ENV on the dev only code. We need to define it in the browsers environment
if (typeof process === "undefined" || !process.env) {
  window.process = {
    env: {
      NODE_ENV: "development",
    },
  };
}

Then we can importing modules just like in the bundling environment.

import * as echarts from "echarts/core.js";
import { PieChart } from "echarts/charts.js";
import { CanvasRenderer } from "echarts/renderers.js";
echarts.use([PieChart, CanvasRenderer]);

But still there are limitations. Because this API design heavliy depends on dead code removal to do the tree-shaking, which seems to be not supported by the browsers yet. So it will still import all the charts from echarts/charts.js, not only the pie chart we needed.

This problem not exists if we use our legacy partially importing API:

import * as echarts from "echarts/lib/echarts";
import "echarts/lib/chart/pie.js";

It will only import the pie chart and all the modules it depends on.

Others

Merging options

  • Please squash the commits into a single one when merging.

Other information

@echarts-bot
Copy link

echarts-bot bot commented Dec 24, 2021

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.

This PR depends on ZRender changes. Please update the ZRender dependency to the latest nightly version including this change, which takes place everyday at 8:00 UTC (16:00 Beijing Time).
You can use npm i zrender@npm:zrender-nightly@dev to update package.json.
If you have any question about this, please leave a comment and we will give you extra help on this.

@pissang pissang changed the title chore: add .js extension in the import statement add .js extension in the import statement Dec 24, 2021
build/pre-publish.js Outdated Show resolved Hide resolved
plainheart
plainheart previously approved these changes Dec 24, 2021
@pissang
Copy link
Contributor Author

pissang commented Dec 24, 2021

@plainheart Just updated zrender to the latest nightly

@pissang pissang merged commit a78ab27 into next Dec 24, 2021
@echarts-bot
Copy link

echarts-bot bot commented Dec 24, 2021

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

@pissang pissang deleted the add-js-extension branch December 24, 2021 07:15
@pissang pissang added this to the 5.3 milestone Jan 4, 2022
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.

2 participants