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 a minified all-inclusive ES6 module build #2530

Closed
wants to merge 8 commits into from

Conversation

TpmKranz
Copy link

I've tried using cytoscape.js as an ES6 module from unpkg.com as follows:

<!doctype html>
<html><body><script type="module">
import cy from 'https://unpkg.com/cytoscape@3.10.0/dist/cytoscape.esm.js?module';
</script></body></html>

This didn't work because, as unpkg.com puts it, Package heap@0.2.6 does not contain an ES module, the same for lodash.debounce@4.0.8.

Apparently, the esm build is not for direct (bundler-less) consumption by an ES6 import.

I'd therefore kindly ask you to include such a build alongside the min one, so that folks can use either the old

<script src="[CDN]/dist/cytoscape.min.js"></script>

or the ES6

import cy from '[CDN]/dist/cytoscape.mes.js';

way of including cytoscape.js.

This PR adds such a build target and calls it cytoscape.mes.js, so as not to interfere with cytoscape.esm.js, which surely should not simply be replaced.

BTW, could someone point me towards the purpose of that esm build compared to, say, the umd build? It seems to me that at least webpack is completely oblivious to the ES6-ness of an imported module.

@maxkfranz
Copy link
Member

.esm.min.js might be a clearer suffix.

BTW, could someone point me towards the purpose of that esm build compared to, say, the umd build? It seems to me that at least webpack is completely oblivious to the ES6-ness of an imported module.

If we expose the esm build as the module field, we break compatibility with existing apps. Say you have an app that uses CJS require(). All your code that calls require('cytoscape') would need to be changed to require('cytoscape').default. I consider that a breaking change, since webpack is so ubiquitous.

Until webpack fixes this issue we can't support module. It would be simple for them to fix it: Just use CJS main files when you're building CJS source, and use ESM module files only when you're building ESM source. As far as I'm aware, I don't think they're made this fix.

You have to configure webpack to alias cytoscape as cytoscape/dist/cytoscape.esm.js if you want to use the ESM dependency.

@maxkfranz
Copy link
Member

Here's a relevant issue: webpack/webpack#4742

@maxkfranz maxkfranz added this to the 3.11.0 milestone Sep 23, 2019
@TpmKranz
Copy link
Author

.esm.min.js might be a clearer suffix.

Of course! I'd only thought of .min.esm.js and rejected that for fear of confusion with the .esm.js build, which serves a completely different purpose. This would also get rid of the necessity of a new target in package.json. MDN also mentions the .mjs extension (yielding .min.mjs), but at least in Ubuntu's nginx and mime-support packages, this extension isn't associated with the required MIME type by default, so I guess this won't be feasible.

Although .esm.min.js still leaves possibility for confusion. Say someone hits an exception and wants to investigate it in the debugger, and therefore tries to import .esm.js instead, assuming that this is the not-minified version of the .esm.min.js build, which it isn't. I've therefore enabled sourcemap generation for that build.

I've also supplemented the documentation a bit, but I'm not quite sure if it's okay to feature the ES6 module so prominently.

Here's a relevant issue: webpack/webpack#4742

Thank you very much for the explanation, now I understand why you bother with a separate esm build.

@maxkfranz
Copy link
Member

Node apparently will consider all .js files as CJS and all .mjs files as ESM. But few people have ever seen .mjs files before. I think the next LTS (12) will officially support ESM (without having to use the esm package, so we'll have to consider this sometime soon.

@TpmKranz
Copy link
Author

I think I found a compromise. There is an ES6 module-version of lodash.debounce and bundling heap with the esm build only adds about 1% file size. Since heap itself doesn't have any dependencies, I don't think it's critical that it remains an external dependency. This way, cytoscape can be used directly from unpkg.com as an ES6 module without a bundler and there is no confusion between the .esm.min.js and .esm.js builds, since they're basically interchangeable.

@TpmKranz
Copy link
Author

Speak of the devil.


```html
<script src="cytoscape.min.js"></script>
<!--Or as an ES6 module-->
Copy link
Member

Choose a reason for hiding this comment

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

The second example should probably be in a separate code block to indicate it's a different example.

The import line shouldn't include the dist part, or people will think they can link directly to the repo.

package.json Outdated
@@ -89,6 +89,7 @@
"main": "dist/cytoscape.cjs.js",
"unpkg": "dist/cytoscape.min.js",
"jsdelivr": "dist/cytoscape.min.js",
"module": "dist/cytoscape.esm.js",
Copy link
Member

Choose a reason for hiding this comment

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

Let's not include the module field until webpack issues are resolved. The documentation could be updated to indicate that an alias needs to be used for ESM support with webpack or rollup.

package.json Outdated
@@ -111,7 +112,7 @@
"watch:build:umd": "cross-env FILE=umd SOURCEMAPS=true NODE_ENV=development rollup -c -w",
"watch:build:cjs": "cross-env FILE=cjs SOURCEMAPS=true NODE_ENV=development rollup -c -w",
"test": "mocha -r esm --recursive",
"test:build": "cross-env TEST_BUILD=true mocha",
"test:build": "cross-env TEST_BUILD=true mocha -r esm",
Copy link
Member

Choose a reason for hiding this comment

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

This should not include -r esm. The point of this test is to test the built bundle as consumed by node. There should be no ES modules in the tested bundle.

package.json Outdated
@@ -158,6 +159,6 @@
},
"dependencies": {
"heap": "^0.2.6",
"lodash.debounce": "^4.0.8"
"lodash-es": "^4.17.15"
Copy link
Member

Choose a reason for hiding this comment

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

Changing to lodash-es may cause issues with CJS apps.

rollup.config.js Outdated
@@ -94,9 +113,10 @@ const configs = [
{
input,
Copy link
Member

Choose a reason for hiding this comment

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

This esm.js bundle should probably be left the same as it is in 3.10

@TpmKranz
Copy link
Author

Thank you very much for your patience, I had gotten a little bit ahead of myself with the esm changes. Sorry!

The documentation now makes a clear distinction between the two scenarios.

@@ -98,7 +98,7 @@
"build:min": "cross-env FILE=min rollup -c",
"clean": "rimraf build/*",
"copyright": "node -r esm license-update",
"dist:copy": "cpy build/cytoscape.umd.js build/cytoscape.min.js build/cytoscape.cjs.js build/cytoscape.esm.js dist",
"dist:copy": "cpy build/cytoscape.umd.js build/cytoscape.min.js build/cytoscape.cjs.js build/cytoscape.esm.js build/cytoscape.esm.min.js build/cytoscape.esm.min.js.map dist",
Copy link
Member

Choose a reason for hiding this comment

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

Let's not include the map file. It's more useful for debugging than production. Many packages don't include sourcemaps for prod, e.g. https://unpkg.com/browse/lodash@4.17.15/

Copy link
Member

Choose a reason for hiding this comment

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

There should also be something like a build:esm.min script

output: {
file: 'build/cytoscape.esm.min.js',
format: 'es',
sourcemap: true
Copy link
Member

Choose a reason for hiding this comment

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

Again, let's turn off the sourcemap

maxkfranz added a commit that referenced this pull request Oct 1, 2019
…ranch

Ref : Add a minified all-inclusive ES6 module build #2530
maxkfranz added a commit that referenced this pull request Oct 1, 2019
Ref : Add a minified all-inclusive ES6 module build #2530
@maxkfranz
Copy link
Member

@TpmKranz I've applied the changes on the unstable branch.

@maxkfranz maxkfranz closed this Oct 1, 2019
@maxkfranz
Copy link
Member

@TpmKranz -- thanks for the PR!

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.

None yet

2 participants