Use rollup to bundle.

[garygreen]

This PR is really just to test the waters and see how you feel about modernising the codebase and using a modern bundler.

With this PR, we can incrementally start modularising functionality and have the ability to tree-shake in the future.

I've left the tabbing level of Sortable.js as-is to avoid doing too much code change, but it can all go in a level now.

As for the main distribution, I think it would make sense to have it inside dist - but obviously changing this path is a pretty big breaking change, so you may prefer to just move Sortable.js to src/sortable.js and keep Sortable.js and Sortable.min.js where they are, so they will be the main bundled/dist code.

Let me know your thoughts.

[owen-m1]

@garygreen Awesome! I will definitely take a look at this and consider it. First I have to look more into rollup. I do think it would be possible to modularize multi-dragging as well looking at the code, but as you said it may take time.

Thanks!

[garygreen]

Excellent. Just for comparison sake, the main alternatives to Rollup are Webpack and Parcel. It seems the general consensus in the community is Rollup should be used for libraries and Webpack for apps. Main reason being that Webpack is a lot more complex, yields larger bundles and main selling feature is code splitting, which often isn't a requirement in bundling libraries. Rollup produces smaller bundles, is much faster to build and generally has a simpler API.

Haven't had much experience with Parcel, but it's generally not as popular as Rollup.

Ref: https://stackoverflow.com/a/43255948/63523

If you need any further info or assistance let me know

[owen-m1]

@garygreen So the idea with rollup here is that I would create a multiDrag.js file, swap.js file, etc., and import them into the main Sortable.js file, and then export them all FROM Sortable.js? And then if the user does not import those specific components from Sortable.js, rollup (or whatever the user's buildtool is) would use tree-shaking and never import the component's file?

[garygreen]

Yes exactly. I've created a repository for you to check out some examples: https://github.com/garygreen/rollup-tree-shake-examples

Noticed this PR has conflicts now, I probably sent it to the wrong branch anyway but if you want me to carry on working this let me know.

[owen-m1]

@garygreen The thing is, the user should still be able to import the plugins as script tags.
For example:

<script src="./Sortable.js"></script>
<script src="./multiDrag.js"></script>

<script>
    Sortable.mount(new MultiDrag());
</script>

If I have everything bundled into one file, then if they use a script tag they will be bringing in all the plugins. I saw that rollup allows for different configurations, though. Perhaps we could go about this having the big Sortable.js (with all the plugins included, and can be tree-shaken) as a sortable.esm.js(?) file, and then also have a Sortable.js file built with rollup, without any of the plugins included, as well as separate multiDrag.js and swap.js files to be used with the latter sortable.js file. Is this feasible and/or practical?

[garygreen]

Yeah well what your suggesting is essentially code splitting. I personally wouldn't bother having different plugin bundles. Folks who use the js with the script tag are probably not caring too much about size of libraries and probably aren't expecting to have much control over the bundle. Those who want to have more finer grain control and import directly (I would suggest) are more likely to expect to have more control, tree shaking etc.

Though it's not unfeasible to have code split modules you can import though. And it can always be added on later anyway.

[owen-m1]

I see your point. Well go ahead and continue with this PR when you have time. I will work on splitting the code into the different files.

[garygreen]

Ok cool. I've updated the PR. Let me know if all looks good. Bare in mind you need to decide you want to do with the main compiled bundles/dist, whether you want to keep it as Sortable.js or if you don't mind having it now go under dist. Or you can just move the Sortable.js file to a new src folder and then use that new folder to start separating modules, etc.

[garygreen]

PS - you probably want to consider removing component.json as well. The project isn't maintained anymore and it seems rather redundant with npm. Maybe all these changes can point towards new major version 2.0 with a migration guide.

[garygreen]

Anymore thoughts on this @owen-m1 ?

[owen-m1]

@garygreen I am still working on splitting the code up. Your changes look good, sorry for not responding.

[garygreen]

Ok. Suprised you'll able to split the code up before this PR is merged, as you need to bundle to test. Unless your saying your mentally preparing for how you would like the code to be split up? Anyway. I'll leave this PR in your hands :-)

[owen-m1]

@garygreen No I cloned your fork.. I don't want to merge until I know it will work out

[owen-m1]

@garygreen I am still not sure about this. I can take out the code for the swap feature relatively easily, but multi-dragging overrides the existing code in so many places that to take it out would create more confusion than it would eliminate.
I had a look at the file sizes - if I minify the current Sortable.js file in the next-version branch, it is only ~6kb greater than the Sortable.min.js on master. Not insignificant, but I don't think it warrants making Sortable plugin-based simply because there are two features which the user may not use. I also plan on making the features more compatible so that you could combine the multiDrag + swap option, and I doubt this would be possible if they were plugins.

If SortableJS was getting rewritten from the ground up, I think plugins would be a great idea, but with the current state the code is in, it would not be very clean.

Let me know what you think.
Thanks.

[garygreen]

That's a pretty heavy size update imo, just for one optional feature that I would argue most apps are not going to be needing. It probably wouldn't stop me using SortableJS, but I do make a conscious decision about the size of libraries we include in our frontend apps because anything that slows download time + parse time = possible loss of SEO / users / money. Sounds drastic, but I do think libraries these days are becoming more and more conscious of how much size burden they are adding to browsers - even the next version of Vue (3.0) which be more modular and is going to reduce the size down in half from 20KB to 10KB. I think these are worthy goals.

Also it's not just about size, it's about code clarity and maintenance - if you have the project split up into well defined modules, it makes it much easier for contributors and general maintenance of the project rather than having one monolithic .js file.

As for the drag module - if this is too challenging to make optional then it'll have to stay in the main bundle, but regardless I still think there's value in using Rollup to bundle stuff and slowly progress towards more modular code. It doesn't have to be done all in one go, incremental steps. Just my two cents 😸

[owen-m1]

You make a good point. The only way then would be for a 2.0 version, something I am wary of doing but I suppose is inevitable anyways. Clearly it isn't as easy as ripping code out and there will need to be a lot of rewritting, but then I could also modularize other components, such as autoscrolling and animations. It could also use ES6+. So it's possible but it will need to be in a 2.0 which will take a lot of time.

[garygreen]

Yeah for sure, will likely target the next major release. I don't mind helping out splitting the code up, but obviously this would need to be merged first / have some kind of bundler. I was also thinking the autoscroller can be a optional thing. Anyway, sounds like your warming to the idea but it's up to you at the end of the day. Thanks for your continued work on this project 👍

[garygreen]

Interesting article: Optimizing JavaScript packages for tree shaking

As an author of (open source) packages, I think you have the responsibility to protect the bundle size of your package consumer. When you publish a package that exports a whole range of modules (for example lodash, ramda, date-fns…) you want to make sure the package is exported in such a way that the consumer of your package (mostly bundlers) can optimize size.

[owen-m1]

@garygreen Exactly what I needed, thanks for sharing

[garygreen]

Any news on this @owen-m1 ? Keen to get this merged and move things forwards. If your still not convinced the benefits of using a bundler then can then close the issue, but I'm still hopeful you can see it's a positive step forwards.

[owen-m1]

@garygreen I have made significant progress on splitting up the code. I also managed to get rollup and minification working, though I have a different configuration than in this PR. It will create an ESM bundle with all plugins included and importable, a UMD with all plugins included, and a UMD of just Sortable and each plugin seperatly (to be jncluded in script tags). So I won't merge this PR, but yes I am working on code splitting with rollup.

[owen-m1]

@garygreen I have now pushed my changes to the v2 branch: https://github.com/SortableJS/Sortable/tree/v2

I have done some code splitting, I created a plugin system and split out the multi-drag and swap features into separate plugins, and I documented the API of the plugin system in it's current state.
I would appreciate it if you could take a look and tell me what you think about the new configuration of the files and how I did the plugin system. Feedback is very helpful, and it is by no means completed yet.
Since I have implemented my own rollup configuration, I will close this PR.

[garygreen]

Looks good. Though at first glance it doesn't appear tree shakeable. For example, in Sortable.js

import { handleAutoScroll, clearAutoScrolls, clearPointerElemChangedInterval } from './Autoscroll.js';

Autoscroll isn't tree shakeable. Though maybe your still working on that aspect?

[garygreen]

Also just as a minor suggestion, I often put imports/exports at the beginning/end of file, rather than mixing them throughout the code. Although it seems more concise to write export function blah() in the file, if all your exports are defined in one place, it makes it much easier to see what the global "api" is for that file.

So for example: https://github.com/SortableJS/Sortable/blob/v2/src/utils.js

I wouldn't be able to tell what utils are available if that was imported without having to go thru all the functions and see. If that makes sense?

[owen-m1]

Autoscroll will be included by default, so no it is not tree shakeable. I figure it is a feature that is necessary for the library to function properly.
I agree with what you are saying about the export statements, and I will make this change.

Thanks for taking a look :)

[garygreen]

Autoscroller - it is a good feature in the library but it's not necessary for the library to function properly. In our app, we actually use another custom made autoscroller library, so we're technically doubling up code there. It works completely independently from other libraries, which means we can use it with other drag libraries.

Also some other things noticed:

• The MultiDrag and Swap plugins are duplicating the utilities, by the looks of it. I would of thought these plugins should only contain the code related to those plugins. So you would be required to install the core Sortable and then MultiDrag, Swap, etc if you need it. Edit: just took another look, it seems to only be duplicating some utility functions like closest userAgent etc, maybe not a big deal but worth keeping an eye on

• The module in package.json points to es dist version which has already been bundled by rollup. I believe webpack can only do tree shaking if it can find exports i.e. it's not already been bundled. So module should ideally point to entry/entry-modular.js

[owen-m1]

@garygreen Ok, I will try to work on making autoscroll a plugin.

Regarding the duplication you noticed, are you seeing this duplication between the dist/plugins/Swap.js and dist/plugins/MultiDrag.js files? It doesn't seem these functions are being duplicated in dist/sortable-complete.js . If it is between the two files, this is to be expected as I bundled them as separate UMDs bringing in their own dependencies. The idea with this is that the user could import them with separate script tags. I don't know any other way of allowing the user to import plugins with script tags except for making them separate UMDs, though I suppose I could make a separate "shared" utils bundle that all the plugins call. Otherwise maybe what you said before is the best option and I shouldn't even create bundles for each plugin.

As for what you said about the module field - I thought the whole point of the esm file was that it is statically analyzable, and can be tree-shaken? If I look in the bundle I can see there are export statements. The "glidejs" library you sent me as an example of a library using rollup also has it's esm bundle set as the module field.

Again, thank you very much for your input.

[garygreen]

My bad, just checked again and although it is bundling, it does look like it's exporting so Webpack knows how to tree shake those functions. Here's some tests and shows size differences between bundles so far:

This was tested using using latest Webpack v4 and just importing:

import { Sortable, MultiDrag, Swap } from '../sortable/dist/sortable.esm';

console.log(Sortable, MultiDrag, Swap);

Only thing I can't work out is why the dist/sortable.min.js version (26KB) is much smaller than the first imported version in the screenshot (31KB). I would of assumed they would be very similar in size, plus the slight additional webpack runtime overhead (around 1KB), but there's a difference of around 5KB which seems odd.

[owen-m1]

@garygreen If you are comparing the current minified version on the master branch to the first output file in your screenshot, it was probably because I had to add the plugin system to the v2 version.

[garygreen]

Ohh ok. Surprised it's that large though tbh as it's mostly just this? It's probably also because of the addition of babel stuff I'm guessing.

[owen-m1]

@garygreen yeah, that and a few functions I made in Sortable,js for the user to interact with Sortable, as well as the actual firing of the events with data. Babel stuff also.

[owen-m1]

@garygreen It is now in the master branch

[owen-m1]

@garygreen FYI - I have released version 1.10.0-rc1 with these changes, and they are backwards compatible (obviously). Backwards compatibility seems like the smoothest way to go for everyone involved : )