Mix of ES6 imports and common.js exports lead to problems with bundling.

[AoDev]

The problem
In some cases, in particular with WebPack, it is not possible to bundle a specific component, like the timeline only for example.

Error that will pop up
TypeError: Cannot assign to read only property 'exports' of object '#<Object>'

Relevant discussions: Webpack issue #4039
Vis is even mentioned in the comments

Some examples in vis code
Timeline line 18 using ES6 import
Timeline line 591 exports as common.js

What should be done
The simple rule is: never mix ES6 with commonjs import/export in the same file. It can work sometimes but just don't take the chance, a few problems can be avoided simply by following this rule. For example resolution of circular dependencies.

Related: There is currently a discussion about linting / code style / ES6, etc where this is particularly relevant as the linter should not allow such mix.

[AoDev]

@mojoaxel What do you think about this? I could send a PR that removes the mix? It's either ES6, either common.js.

[wimrijnders]

So the general rule, transcending ES6, would be: "use require statements instead of import".
Did I understand it correctly like this?

[AoDev]

Not exactly. You have to decide what kind of module it should be. Depending on how you import / export, babel and webpack will interpret it differently and mixing can cause problems.

You should not do things like:

// myModule.js

import x from 'module-x'
...

module.exports = something

instead:

common.js way

// myModule.js

const x = require('module-x')
// Here there is a trick, if module-x is exported as ES6 with a default export it would be
const x = require('module-x').default
...

module.exports = something

or

ES6 way

// myModule.js

import x from 'module-x'
...

export default something

I recommend the ES6 way because it solves things like circular dependencies and you can do tree shaking. Some libraries are published with both versions: ES6 and transpiled dist. So people can import the ES6 source directly.

[wimrijnders]

OK, so either the first way or the second way, but don't mix the two.
This could definitely use some eslint support, as you suggested.

Looking at network, I see it uses both ways. I suppose if one of the ways is used exclusively per file it will work.
Looking at graph3d, I see it uses the common.js way exclusively.

Would you think changing them completely to the ES6 way is a good idea?

[AoDev]

Right now there is a discussion about code style which I linked above. One of the points is using ES6 (although it's not really a "style"). I'd rather have people decide on this. Because even if personally I would use the ES6 import / export syntax everywhere, it could be decided after that "no ES6" and we would have to change them all.

[wimrijnders]

Clear. I would personally prefer not to exclude ES6, rather have it as an upgrade path. I'm of the opinion that eslint:recommended is a good place to start (your suggestion in the linter discussion). But I digress.

[patsissons]

what is the status on this? might be easier for now to just fix the issue by replacing the ES6 imports with CJS require calls to resolve the immediate issue and then discuss further on an upgrade to ES6 imports. I didn't fully scan the sources, but it seems like the majority of imports are CJS style, which is why I'm suggesting the CJS patch.

[mojoaxel]

I'm really not happy that #3063 was merged! All the new code is written in ES2015 and I thought this is the way to go!? I don't understand why we are going back to CommonJS now? For me this makes no sense, we are not even using RequireJS. If something should be changed than to the newer Standard and not a obsolete on!!

@AoDev I'm integrating the timeline in my webpack project without any problems. Webpack is able to mix and match different module types.

Not happy 😞

[patsissons]

@mojoaxel this is not a progressive change, it is a temporary bandage to fix broken code so that webpack is properly supported. Not all files were broken, only the modified files were broken. If you were not importing those broken modules with webpack then you it would appear as if there was no issue. The changes made were simply the path of least resistance to fully support webpack.

Now that #3063 is merged, focus csn be put on figuring out a timeline to migrate all code to ES6 imports without simultaneously impacting consumers of vis.

[AoDev]

@mojoaxel If you read the comments in the issues linked above, you should have seen that it will fail in some cases. Happy it works for your setup.

Regarding ES6 / common.js there is an open poll that has been around for a long time. The maintainers should take a decision and put this in a linter that would have rejected the PR...