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

feat(build): add standalone and peer builds #85

Merged
merged 42 commits into from
Oct 27, 2019
Merged

feat(build): add standalone and peer builds #85

merged 42 commits into from
Oct 27, 2019

Conversation

Thomaash
Copy link
Member

@Thomaash Thomaash commented Aug 20, 2019

We need the same data set to be used in all modules. The way it is now is disfunctional for multiple modules used at the same time. For example in UMD the network data set will be overwritten by the timeline data set causing network to not recognize passed data sets as data sets and throw an error (this is simply broken at the moment, no way around this). In ESM the bundled data set is used and the user has to take great care to use the network data set for network and the timeline data set for timeline etc. Data set from vis-data package won't work anywhere. The same applies to data view.

The original files including module and main props in package.json are kept for backwards compatibility (they work just fine when only one module is used on it's own). However maybe it would be better to make this a breaking change to have peer version as default (good for npm packages and multiple module usage) and standalone as a convenience build for single module projects, JSFiddle etc.

To do

  • Discuss.
  • What to do with DOMUtil (we don't use it at all in Vis Network)?
    • Removed in the new builds, bundled in legacy.
  • What to do with Moment (we don't use it during runtime in Vis Network)?
    • Removed in the new builds, bundled in legacy.
  • Thoroughly test.
  • Create examples.
  • Create documentation.
  • Apply the same solution to the other modules.

Solutions that do not work

  • module.exports = window.vis.DataSet || require('vis-data').DataSet

    • There is no way how to make this work in ESM build: imported modules do not leak into the global scope.
    • This creates stack overflow in UMD builds: for example network (vis namespace) requires data (vis namespace), in other words vis has to be loaded in order to load vis.
    • This is used for Moment and Hammer but for these it works only in UMD builds.
    • A quite minor issue here is that the modules are bundled even if they'll never be used (aka dead code).
  • Using the source code instead of builds.

    • Works for charts module.
    • Very inconvenient for users.
    • Also we should not ship source codes (and other junk) as part of the package, only ready to use built files.

Closes #57, closes #97, closes #123, closes #166.

@Thomaash Thomaash requested review from mojoaxel, yotamberk and a team August 20, 2019 19:03
@Thomaash
Copy link
Member Author

Linting fails because @typescript-eslint/member-ordering rule doesn't work well, disabled through #93.

Also ditches rollup-plugin-babel-minify in favor of directly configuring
babel plugin as the former doesn't work with core-js 3.
@Thomaash
Copy link
Member Author

Thomaash commented Sep 2, 2019

Hi @mojoaxel,

do you have any plans to review this? I need to update to core-js 3 to resolve #57. This already uses core-js 3 so merging this would save me some work. Plus I could rewrite the build process this way everywhere to also resolve visjs/vis-timeline#67.

@Thomaash Thomaash mentioned this pull request Sep 2, 2019
@mojoaxel
Copy link
Member

mojoaxel commented Sep 3, 2019

do you have any plans to review this?

To be honest, I'm a little overwhelmed. I have problems fully understanding this PR and all the effects it may have.

  • What is corejs: 3?
  • All this is definitely a breaking change because existing dist files do not exist anymore. People don't like breaking changes 😉
  • It looks to be this change would need a lot of documentation on wich bundles exist and when to use them. This documentation is missing.
  • What is the difference between standalone and standalone-bundle?
  • I would prefer having named bundles including vis-network everywhere e.g. vis-network.umd.peer.min.js
  • if DOMUtil is really not used(?) in it should be removed!
  • We propably would need to adopt all the examples to the new bundles!?

@Thomaash
Copy link
Member Author

Thomaash commented Sep 3, 2019

  • core-js provides polyfills for new features so that we can use them and still be compatible with old browsers.
  • This is not a breaking change because the new standalone builds (which are the same as the old builds) are also copied to their former locations for backwards compatibility.
  • I'll add documentation this to the check list.
  • Standalone contains all the named exports but no default export, standalone bundle takes the named exports, reexports them and also default exports all of them as a single object. This is so that we don't break builds to people who import like this: import vis from 'vis-network'. It could be done without this file but then we would have to manually ensure that import * as vis from 'vis-network' actually gives the same result as import vis from 'vis-network' which would be error prone.
  • The idea behind vis-network free names is that we can have the same names everywhere. That is imports like:
import { DataSet } from 'vis-data/dist/esm.peer.js'
import { Network } from 'vis-network/dist/esm.peer.js'
import { Graph3d } from 'vis-graph3d/dist/esm.peer.js'

With package prefixes it would work too of course, no problem there, it just seems a bit redundant to me:

import { DataSet } from 'vis-data/dist/vis-data.esm.peer.js'
import { Network } from 'vis-network/dist/vis-network.esm.peer.js'
import { Graph3d } from 'vis-graph3d/dist/vis-graph3d.esm.peer.js'

Although this would maybe be a bit easier for people who copy paste these files into their projects as they wouldn't conflict with each other.

  • Removing DOMUtil would be a breaking change but since we don't use it we should remove it. The same goes for Moment which is also used solely in Timeline.
  • As it is at the moment this PR is 100 % backwards compatible, so the examples will work the way they are.

@r3code
Copy link
Contributor

r3code commented Sep 8, 2019

https://github.com/visjs/vis-network/blob/7c758785819f12905ae0512e10243ef2763fd0e1/lib/header.js uses moment.js only to create header. Maybe we just drop it and create string UTC date inline?

@Thomaash
Copy link
Member Author

Thomaash commented Sep 9, 2019

The question isn't how to replace Moment in header.js, that's not part of runtime, that's only part of build process. The question is whether we should continue bundling and exporting Moment for backwards compatibility or release a breaking change without it.

@r3code
Copy link
Contributor

r3code commented Sep 9, 2019

@Thomaash For me the new visjs split by modules looks like completely new and I expect that it can be incompatible with the old vis.js

@Thomaash
Copy link
Member Author

Thomaash commented Sep 9, 2019

If we haven't released any version of Vis Network yet this would be a valid argument. However now it's not about compatibility with the original Vis, it's about compatibility with the Vis Network versions we've already released.

@mojoaxel
Copy link
Member

@Thomaash For me the new visjs split by modules looks like completely new and I expect that it can be incompatible with the old vis.js

To prevent this we created vis-charts. The idea was to have something like a plug&play replacement for people who have no idea what they are actually doing.
I also encourage that our new libraries feel more like a npm-module and less like a jquery-plugin 😉

Copy link
Member

@mojoaxel mojoaxel left a comment

Choose a reason for hiding this comment

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

Im not sure if I stayed it before. But I'm not a big fan of this generic filename. It's really hard to debug files that are just calles "esm.js" or "esm.min.js". Please add the a prefix e.g. "vis-timeline.esm.min.js" to all files in the future.
Here is an example of the problem i'm just having in IE11:
image

@r3code
Copy link
Contributor

r3code commented Sep 16, 2019

Im not sure if I stayed it before. But I'm not a big fan of this generic filename. It's really hard to debug files that are just calles "esm.js" or "esm.min.js". Please add the a prefix e.g. "vis-timeline.esm.min.js" to all files in the future.

Yeah/ THis is really bad thing. It's like common, utils, all, other.js horrible with no meaning names.

@Thomaash
Copy link
Member Author

My idea was something like:

import { Network } from 'vis-network/peer.esm'

This will give you:

import { Network } from 'vis-network/vis-network.peer.esm'

Seems pretty idiotic from user's point of view. On the other hand opening dev tools and seeing some mysterious esm.js or whatnot sucks. However this is only a problem if source maps fail as source maps would give you the actual source file it came from. Atop of that you can get the full path (at least in Chrome but this seems like an of course thing to me) from the tooltip of the tab and in the sidebar (unless you close it of course). Not ideal but still there.

We could make this a breaking change and have the peer build default. This would import directly from vis-netwrok without any filenames. With the standalone URL being something like https://unpkg.com/vis-network@7.0.0/vis-network.standalone.umd.js. Still seems pretty stupid but this doesn't make much of sense unless it's included in <script/> in the header (like JSFiddle or some old school page). Users wouldn't import this over and over again in several files like with the ESM peer build. This would get rid of most of my objections to having the name of the library in the file name.

Another variation is having both. With the name of the library for use directly in browsers and without it for use with bundlers. Though it's questionable whether people would follow this pattern or just get confused on what is the difference between these identical files.

@Thomaash
Copy link
Member Author

Okay so the examples are in place, the docs are updated and I tested it in vis-charts (it works 🎉, we no longer have to build Network from source files). So @mojoaxel and @yotamberk this is ready for review and if you won't find any bugs I'll start preparing this for the other packages.

This ensures that all dependencies match our browser list.
rollup.build.js Outdated Show resolved Hide resolved
@mojoaxel
Copy link
Member

mojoaxel commented Oct 1, 2019

Thanks @Thomaash for your work here! This is one of the biggest changes we had in since splitting up the library and I really would like to ask @yotamberk and the other @visjs/maintainer to think about if this is the future we want to go with the visjs libraries,

During the almende/vis times we basically had ONE single JS file and one CSS file that the user could add to his HTML. If this PR gets merged we would have the following:

image

I see that there may be use-cases for every of these bundles but I'm not sure if this is practical and the right way to go. But I'm also not an expert or something...
We should discuss this in the community. I'm not yet willing just to approve this, alone!

@Thomaash
Copy link
Member Author

Thomaash commented Oct 1, 2019

Well to put it shortly:

  • /dist/ for backwards compatibility
  • /peer/ so that different packages (for example vis-network and vis-timeline) can be used together on the same page without issues
  • /standalone/ to provide convenient build when only one package is needed like in examples or JSFiddle (everything including CSS is bundled in single file)

All exist in UMD and ESM versions, have accompanying d.ts entry files ("files" property of package.json in case of /dist/) and source maps.

This greatly increases compatibility as Array.prototype.flatMap is quite
new and only recently supported by Node.
mojoaxel
mojoaxel previously approved these changes Oct 27, 2019
Copy link
Member

@mojoaxel mojoaxel left a comment

Choose a reason for hiding this comment

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

I still don'z like this change. It looks to me like it over-complicates thinks and I'm sure it's making it even harder to use this library for normal users.

@Thomaash Can you point to any other project who follow this approach of supporting multible bundles?

Anyway I see no other way to solve the bundling issues other than adding the sources to the distributed package. I don't want to stand in the way of progress so here is my approval!

@Thomaash
Copy link
Member Author

For example Vue.js has several variants in it's package https://unpkg.com/browse/vue@2.6.10/dist/.

@Thomaash
Copy link
Member Author

I had to merge the changes from the master (there was a lot of it).

@Thomaash Thomaash merged commit 4e0d998 into visjs:master Oct 27, 2019
@Thomaash Thomaash deleted the peer-build branch October 27, 2019 14:50
@vis-bot
Copy link
Collaborator

vis-bot commented Oct 27, 2019

🎉 This PR is included in version 6.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants