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

dev-build: update esbuild #16619

Closed
wants to merge 3 commits into from
Closed

dev-build: update esbuild #16619

wants to merge 3 commits into from

Conversation

susiwen8
Copy link
Contributor

@susiwen8 susiwen8 commented Mar 3, 2022

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Since esbuild decide to improve compatibility with Webpack and Node. After version 0.14.4, esbuild adjust the way of handling of default exports and the __esModule marker

https://github.com/evanw/esbuild/releases/tag/v0.14.4

Fixed issues

Details

Before: What was the problem?

After: How is it fixed in this PR?

Misc

  • The API has been changed (apache/echarts-doc#xxx).
  • This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

N.A.

Others

Merging options

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

Other information

@echarts-bot
Copy link

echarts-bot bot commented Mar 3, 2022

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.

// CommonJS
factory(exports);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change exports to module may behave differently in the CommonJS environment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the UMD header, it may be better to use the same block with rollup.

(function (global, factory) {
    typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports) :
    typeof define === 'function' && define.amd ? define(['exports'], factory) :
    (global = typeof globalThis !== 'undefined' ? globalThis : global || self, factory(global.echarts = {}));
}(this, (function (exports) {
    // ...
})));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why I have change exports to module is that esbuild generate module.exports = ___toCommonJS(****) at the end of file if we chose cjs format.
Cause in browser environment, module is not a global variable as in node. It will throw module is not defined in browser.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plainheart Well, Same error if we use rollup header

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JS spec is not my area of expertise. To be honestly, I'm still confused by esbuild

@@ -142,7 +142,7 @@
var factoryRet = factory.apply(null, deps.map(function (dep) {
return dep === exportsPlaceholder ? modExports : dep;
}));
return factoryRet || modExports;
return factoryRet || modExports.exports || modExports;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain this change to me? I think this is not same with the AMD spec

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, modExports would be an argument for factory function, as matter of fact, variable modExports will be variable module when function runs. Then as I have mention above, The exports attribute of module will have whatever echarts have exported.

modExports = {
    exports: echarts.
}

@pissang
Copy link
Contributor

pissang commented Mar 4, 2022

package-lock.json needs to be updated

@susiwen8
Copy link
Contributor Author

susiwen8 commented Mar 4, 2022

package-lock.json needs to be updated

@pissang I'm concerned about package.lock.json. Casue my lockfile version is not same with previous. I remember @plainheart have disscuess with you.

@pissang
Copy link
Contributor

pissang commented Mar 4, 2022

@Ovilia has updated the package-lock.json to 2.0 in the release branch. Perhaps we can leave this PR to be merged after merging release to master

@susiwen8
Copy link
Contributor Author

susiwen8 commented Mar 4, 2022

@Ovilia has updated the package-lock.json to the latest 2.0 in the release branch. Perhaps we can leave this PR to be merged after merging release to master

Ok

@pissang
Copy link
Contributor

pissang commented Mar 7, 2022

package-lock.json in the master branch has been updated.

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