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

fix(chore): fix main file location, ensure compatibility with nodejs #842

Closed
wants to merge 1 commit into from
Closed

fix(chore): fix main file location, ensure compatibility with nodejs #842

wants to merge 1 commit into from

Conversation

3cp
Copy link

@3cp 3cp commented Aug 1, 2018

The current setup uses implicit default main file index.js which uses es6 native module import syntax, and directly loads from source file (lib folder) which uses native module import syntax too.

The whole setup bypasses dist file prepared by rollup.

The reason that this setup still works in webpack, is webpack supports es6 native module import syntax. We don't have the luck if we use the main file in nodejs or browser environments directly (both currently do not support import syntax by default).

The current setup uses implicit default main file `index.js` which uses es6 native module `import` syntax, and directly loads from source file (`lib` folder) which uses native module `import` syntax too.

The whole setup bypasses `dist` file prepared by rollup.

The reason that this setup still works in webpack, is webpack supports es6 native module `import` syntax. We don't have the luck if we use the main file in nodejs or browser environments directly (both currently do not support `import` syntax by default).
@ghost ghost added the needs review Review pending label Aug 1, 2018
@nikku
Copy link
Member

nikku commented Aug 1, 2018

What is your setup / which setups would you like to support?

@3cp
Copy link
Author

3cp commented Aug 1, 2018

The linked issue above uses aureli-cli which uses a requirejs based build, it supports normal commonjs/umd/amd code which is shipped by most npm packages.

The current setup stops nodejs with a simple var BpmnJS = require('bpmn-js');.
But var BpmnJS = require('bpmn-js/dist/bpmn-viewer.production.min'); works fine after properly preparing jsdom.

@nikku
Copy link
Member

nikku commented Aug 1, 2018

I guess aurelia is using browserify to consume libraries and bundle things for the browser.

Did you consider adding the respective babelify transform to translate ES module import / export declarations to cjs?

@nikku
Copy link
Member

nikku commented Aug 1, 2018

The alternative is to use the workaround you provide.

@3cp
Copy link
Author

3cp commented Aug 1, 2018

Not browserify, aurelia uses amodra-trace currently which translate cjs into amd module.
It does babel on source code but skipped all npm modules as it assume all npm modules provided code understandable by nodejs.

I think it's a common practice to ship npm package in a format which is understandable by nodejs. After all, npm package is designed to be consumed by nodejs.

This PR would not affect webpack, just reduce its effert in dealing with import and bundling multiple files which is already done by rollup.

@3cp
Copy link
Author

3cp commented Aug 1, 2018

I understand, this is not a big issue for using bpmn in aurelia. After all, it's your call whether to make it smoother to nodejs env and other bundlering tool other than webpack.

@nikku
Copy link
Member

nikku commented Aug 1, 2018

After all, it's your call whether to make it smoother to nodejs env and other bundlering tool other than webpack.

We came from CommonJS and switched to ES modules for reasons such as smaller bundle sizes / tree shaking. My guess is that eventually, all decent bundlers and NodeJS will understand ES modules, as it is the new standard for JavaScript modules.

Before we reach this point, people that are forced into AMD / cjs environments with no ability to control the actual transpilation should resort to the workaround you mentioned:

var BpmnJS = require('bpmn-js/dist/bpmn-viewer.production.min') works fine after properly preparing jsdom.

@nikku nikku closed this Aug 1, 2018
@ghost ghost removed the needs review Review pending label Aug 1, 2018
@nikku nikku added the wontfix label Aug 1, 2018
@3cp
Copy link
Author

3cp commented Aug 1, 2018

Thx for the reasoning. Appreciated.

@3cp 3cp deleted the fix-main-path branch August 1, 2018 12:22
@nikku nikku added the wontfix This will not be worked on label Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants