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

Basic convert to esm with only default exports #2084

Merged
merged 3 commits into from Apr 12, 2018

Conversation

Projects
None yet
2 participants
@TrySound
Copy link
Contributor

commented Mar 26, 2018

Ref #2012

I decided to make at first basic conversion with only default exports. Also I added esm loader.

@TrySound

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2018

before

cytoscape.js 815.1K
cytoscape.min.js 325.2K
cytoscape.umd.js 795.9K

after

cytoscape.js 888.0K
cytoscape.min.js 327.0K
cytoscape.umd.js 868.8K

seems like currently size increased with 2kb, but it should be fixed with rollup's scope hoisting

@TrySound

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2018

Benchmarks has a lot of lint errors so I'd like to convert them later.

@maxkfranz maxkfranz self-requested a review Mar 26, 2018

@TrySound

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2018

@maxkfranz Thoughts?

@maxkfranz

This comment has been minimized.

Copy link
Member

commented Apr 11, 2018

It looks OK from what I saw before. I just haven't had time to test and merge...

I'll try to set aside some time tomorrow to try it out (and possibly merge).

@maxkfranz maxkfranz self-assigned this Apr 11, 2018

@maxkfranz maxkfranz added this to the 3.3.0 milestone Apr 12, 2018

@maxkfranz maxkfranz merged commit 323397a into cytoscape:unstable Apr 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maxkfranz

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

Some questions re. next steps:

(1) Should we be specifying 'module' in package.json? http://2ality.com/2017/07/npm-packages-via-babel.html

(2) Does Rollup support everything we're currently doing with Webpack in the webpack.config.js and the package.json?

@maxkfranz

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

Re (2), we need:

(a) Watch support : --watch via CLI: https://rollupjs.org/guide/en#command-line-flags

(b) Inline source maps toggled on/off via config file: https://rollupjs.org/guide/en#output-sourcemap-m-sourcemap

(c) A way to minify with Uglify and toggle it on/off with an env var in the config file: Not sure if this is possible. We could just pipe the output to uglify, but then Windows users can no longer build. For myself, that's not a problem but it might discourage Windows users from contributing to the lib.

(d) Babel support: https://rollupjs.org/guide/en#babel

(e) Making a UMD build that supports external refs to dependencies. Not sure if this is supported.

(f) Bundling dependencies from npm that aren't ES modules: It looks like a plugin does this https://rollupjs.org/guide/en#npm-packages

(g) Something analogous to EnvironmentPlugin from Webpack : Not sure if this exists

@TrySound

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2018

  1. Yes. module field will make sense when we will land esm bundle.
  2. Yes. Rollup support everything we need.
    a. rollup-watch
    b. yes
    c. There is cross-env package which works for all shells.
    d. rollup-plugin-babel
    e. yes
    f. rollup-plugin-node-resolve and rollup-plugin-commonjs
    g. rollup-plugin-replace

@TrySound TrySound deleted the TrySound:basic-esm branch Apr 12, 2018

@TrySound TrySound referenced this pull request Apr 30, 2018

Merged

Bundle with rollup #2098

@maxkfranz maxkfranz modified the milestone: 3.3.0 Nov 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.