ES6 modules & Rollup #4989

mourner · 2016-09-30T20:28:08Z

This an in-progress PR that switches Leaflet to use ES6 modules system and Rollup for bundling. Closes #3229. It's A LOT of tedious work going through all the code, but it looks like it's going to be a major improvement.

Dependencies will be imported explicitly in each file instead of depending on global namespaces, making the code easier to maintain and reason about.
Things like L.Util.bind will turn into just bind, which means pretty much all references to Leaflet functions/classes will be properly minified, reducing the bundle size significantly (in theory).
I love the syntax, it just comes along very naturally.
We'll switch to a rollup-watch-based debug workflow, eliminating the document.write hack.
People who use ES6 modules or Rollup in their apps will be able to benefit from tree shaking (import stuff they need directly and discard all the rest).

cc @IvanSanchez @patrickarlt @olanod @tmcw @perliedman @yohanboniface @hyperknot

Status:

tmcw · 2016-09-30T20:35:29Z

src/map/Map.js

+import {LatLngBounds, toLatLngBounds} from '../geo/LatLngBounds';
+
+import {
+	any3d as isAny3D,


at least within iD and Studio, we've tried to avoid as statements so that it's easier to track down code across the whole codebase. Minor nitpick, and not necessarily something you want to adopt, but I think it's worked well.

Yeah, I'd like to avoid as too, but the problem is that without namespaces, some of the exported names become awkward, and I wanted each file to export names that will end up in the namespace. See a397b63 for how the Browser module looked, for example. Maybe we'll want to migrate to new names, but in a breaking way, in future.

mourner · 2016-10-04T11:19:10Z

src/geo/crs/CRS.Base.js

@@ -15,7 +15,7 @@ import {wrapNum} from '../../core/Util';
 * [Proj4Leaflet](https://github.com/kartena/Proj4Leaflet) plugin.
 */

-export var Base = {
+export var Base = extend({}, {


Why extend an empty object here here?

Nevermind that, I was confused by eslint throwing an error like A function with a name starting with an uppercase letter should only be used as a constructor new-cap, but did not notice it was due to misuse of the Bounds constructor

mourner · 2016-10-04T11:20:11Z

package.json

@@ -29,7 +29,8 @@
    "test": "jake test",
    "build": "jake build",
    "release": "./build/publish.sh",
-    "rollup": "rollup src/Leaflet.js -f umd -n L > bundle.js"
+    "lint": "node_modules/.bin/eslint --fix src/",


node_modules/.bin is not necessary in package.json scripts — it's in path automatically.

Somehow my Linux setup doesn't work like that. 😩

@IvanSanchez so eslint doesn't work but rollup and jake do? That's weird.

In my case, I put rollup and jake manually in my $PATH because I was tired of running them locally (but they have to be updated/upgraded separately). Not so with eslint.

Well this is weird, it shouldn't be like that... But I'm sure others had a problem like this, should be googleable...

nathancahill · 2016-10-04T16:59:01Z

I have the same experience. ./node_modules/.bin has to be specified to take priority over globally installed executables.

Scarysize · 2016-10-19T14:50:28Z

build/rollup-config.js

+
+import gitRev from 'git-rev-sync'
+
+// TODO: There should be a way to skip the git branch+rev when doing a production build


process.env.NODE_ENV or a rollup production configuration would do the trick.

Yeah, but I'll wait for @mourner's input on the issue. This will affect the build/publish.sh script.

Will it? As far as I see, gitRev is only needed for the L.version property and copyright comment, and publish.sh takes the clean semver version from package.json.

What I mean is that publish.sh will probably need a third rollup config file, in order to skip the git rev.

We can either set an env var (read from the config), or just leave git rev in the copyright and L.version — no big deal.

mourner · 2016-10-20T12:48:10Z

src/Leaflet.js

@@ -30,7 +31,7 @@ export {Class} from './core/Class';

 import {Evented} from './core/Events';
 export {Evented};
-export var Mixin = {Events: Evented.prototype};
+// export var Mixin = {Events: Evented.prototype};


Accidental? I did this for backwards compat.

@mourner I was not aware this was a backwards-compat issue, sorry 'bout that :-(

IvanSanchez · 2016-10-20T12:59:46Z

I tried to make some clever use of rollup in the browser, but hit a lot of roadblocks. Thus, rely on npm run-script watch or yarn run watch for the time being. The script includes a hack in the JS banner so devs are reminded to use a fresh bundle.

mourner · 2016-10-21T14:00:04Z

Let's rebase to fix the Canvas conflict, and also maybe ditch all uses of as in favor of importing namespaces (which, as @IvanSanchez showed, are still neatly unrolled by rollup).

IvanSanchez · 2016-10-21T14:06:54Z

@mourner Rebased. I'll take a break from this branch, and will let someone else rewrite the named DomUtil, Util and DomEvent imports.

mourner · 2016-10-25T15:12:26Z

@IvanSanchez BTW what's up with the unrunnable Path tests?

IvanSanchez · 2016-10-25T15:15:33Z

@mourner L.Path is no longer exported (because it should be something like an un-instantiable abstract class), thus the tests involving new L.Path cannot work.

Maybe we should export L.Path as well, but I'm unsure.

IvanSanchez · 2016-12-23T12:06:25Z

After some rebasing hell and some fiddling with the VML code, the IE8 builds work again.

mourner · 2017-01-25T13:43:19Z

docs/index.html

@@ -5,7 +5,7 @@
 <div class="announcement">Jan 23, 2017 — <a href="http://leafletjs.com/2017/01/23/leaflet-1.0.3.html">Leaflet 1.0.3</a>, a bugfix release, is out.</div>

 <p>Leaflet is the leading open-source JavaScript library for mobile-friendly interactive maps.
-Weighing just about <abbr title="33 KB gzipped &mdash; that's 123 KB minified and 218 KB in the source form, with 10 KB of CSS (2 KB gzipped) and 11 KB of images.">33 KB of JS</abbr>,
+Weighing just about <abbr title="38 KB gzipped &mdash; that's 133 KB minified and 378 KB in the source form, with 10 KB of CSS (2 KB gzipped) and 11 KB of images.">38 KB of JS</abbr>,


Whoa, is leaflet-src.js really 378kb?

Yes, because we added docstrings to everything, and that kinda doubles the size of the file.

Oh, I forgot about that :)

per@linfro:~/Documents/Projects/Leaflet$ npm run build > leaflet@1.0.3 build /home/per/Documents/Projects/Leaflet > jake build Bundling and compressing 67 files... Uncompressed: 372.64 KB (new) Saved to dist/leaflet-src.js Compressed: 141.92 KB (new) Saved to dist/leaflet.js Gzipped: 37.97 KB

perliedman

I've reviewed this by actually trying to use this branch in a pretty large Leaflet based application. The application uses quite a few plugins as well.

From what I can tell, this branch now works with this application, I can't notice any differences between this an Leaflet 1.0.2, which we used before.

I think we need a more proper code review as well, but this at least indicates there are no obvious gotchas if we merge this.

simon04

Adding index.js files to subdirectories/namespace allows to streamline the import/export in the main file. I commented on a few lines with specific suggestions.

simon04 · 2017-01-26T20:16:42Z

src/Leaflet.js

-} else if (typeof define === 'function' && define.amd) {
-	define(L);
-}
+export {Control, control};


Adding a file ./control/index.js and doing the preparation of Control/control there would streamline the code in Leaflet.js down to

export {Control, control} from './control';

simon04 · 2017-01-26T20:20:04Z

build/build.js

-
-	console.log('Bundling and compressing ' + files.length + ' files...');
-
-	var copy = fs.readFileSync('src/copyright.js', 'utf8').replace('{VERSION}', version),


The file copyright.js can be removed.

simon04 · 2017-01-26T20:22:15Z

src/Leaflet.js

+import {TouchZoom} from './map/handler/Map.TouchZoom';
+Map.TouchZoom = TouchZoom;
+
+export {Map, createMap as map} from './map/Map';


Adding a file ./map/index.js and doing the preparation of Map there would streamline the code in Leaflet.js down to

export {Map, map} from './map';

simon04 · 2017-01-26T20:24:15Z

src/Leaflet.js

+import {TileLayerWMS, tileLayerWMS} from './layer/tile/TileLayer.WMS';
+TileLayer.WMS = TileLayerWMS;
+tileLayer.wms = tileLayerWMS;
+export {TileLayer, tileLayer};


Adding a file ./layer/tile/index.js and doing the preparation of TileLayer/tileLayer there would streamline the code in Leaflet.js down to

export {GridLayer, gridLayer, TileLayer, tileLayer} from './layer/tile';

simon04 · 2017-01-26T20:28:52Z

src/Leaflet.js

+GeoJSON.latLngsToCoords = latLngsToCoords;
+GeoJSON.getFeature = getFeature;
+GeoJSON.asFeature = asFeature;
+export {GeoJSON, geoJSON, geoJson};


Maybe add the static functions to the GeoJSON class in ./layer/GeoJSON.js? Or prepare and export a GeoJSON_with_static_functions object in ./layer/GeoJSON.js?

simon04 · 2017-01-26T20:29:43Z

src/Leaflet.js

+CRS.EPSG4326 = EPSG4326;
+CRS.Simple = Simple;
+
+export {CRS};


Adding a file ./geo/crs/index.js and doing the preparation of CRS there would streamline the code in Leaflet.js down to

export {CRS} from './geo/crs';

simon04 · 2017-01-26T20:32:27Z

src/Leaflet.js

+
+// geo/projection
+
+import * as Projection from './geo/projection/Projection';


Rename Projection.js to index.js to comply with other suggestions and the node.js module import.

simon04 · 2017-01-26T20:34:53Z

src/Leaflet.js

+export {Icon};
+
+export {DivIcon, divIcon} from './layer/marker/DivIcon';
+export {Marker, marker} from './layer/marker/Marker';


Combine all markers in ./layer/marker/index.js and export them here all at once:

export{Icon, icon, DivIcon, divIcon, Marker, marker} from './layer/marker';

simon04 · 2017-01-26T20:36:57Z

src/Leaflet.js

+export {Circle, circle} from './layer/vector/Circle';
+export {Polyline, polyline} from './layer/vector/Polyline';
+export {Polygon, polygon} from './layer/vector/Polygon';
+export {Rectangle, rectangle} from './layer/vector/Rectangle';


Combine all vector layer classes in ./layer/vector/index.js and export them here all at once:

export{Renderer, Canvas, canvas, SVG, svg, Path, CircleMarker, circleMarker, Circle, circle, Polyline, polyline, Polygon polygon, Rectange rectangle} from './layer/vector';

IvanSanchez · 2017-01-27T08:38:52Z

@simon04 You make a few good points! As far as I'm aware, Rollup (et al) can tree-shake better if the modules are not assigned, but just imported/exported.

However, right now I think it'd be better to just merge this as it is, and worry about cleaner ES6 module exports later.

As long as the ES5 bundle works exactly the same, all that can be postponed.

mourner added the in progress label Sep 30, 2016

tmcw reviewed Sep 30, 2016

View reviewed changes

yohanboniface mentioned this pull request Oct 2, 2016

Inconsistent EOL-style in the source code #4996

Open

IvanSanchez mentioned this pull request Oct 4, 2016

Use rollup-plugin-buble and use shorthand ES6 class methods #5002

Closed

mourner commented Oct 4, 2016

View reviewed changes

IvanSanchez mentioned this pull request Oct 12, 2016

WIP: ES6-ify more stuff #5013

Merged

IvanSanchez mentioned this pull request Oct 19, 2016

Getters/setters should be removed somehow futurist/rollup-plugin-es3#3

Open

Scarysize reviewed Oct 19, 2016

View reviewed changes

IvanSanchez force-pushed the rollup2 branch from 1e2ff42 to 1f1c6c5 Compare October 19, 2016 14:55

mourner commented Oct 20, 2016

View reviewed changes

IvanSanchez force-pushed the rollup2 branch from 1f1c6c5 to 8759ab2 Compare October 21, 2016 14:05

IvanSanchez force-pushed the rollup2 branch from c5aa543 to c78384b Compare October 24, 2016 15:40

IvanSanchez force-pushed the rollup2 branch 2 times, most recently from 792e3ea to 1b124f0 Compare November 3, 2016 08:42

IvanSanchez force-pushed the rollup2 branch from eceb2ea to da094c6 Compare December 9, 2016 10:38

IvanSanchez force-pushed the rollup2 branch 2 times, most recently from 1a686a6 to addaa88 Compare December 23, 2016 11:56

mourner added 5 commits January 25, 2017 13:03

WIP ES6 modules & rollup

d8af31c

WIP ES6 modules & rollup 2

7118aae

WIP ES6 modules & rollup 3

6068503

WIP ES6 modules Browser

e759342

WIP ES6 module fixes

4864a9a

IvanSanchez added 7 commits January 25, 2017 13:30

ES6ify: Use rollup's "legacy" option for ES3 (IE8) builds

037b1a9

ES6-ify: Clean up build scripts, fix CONTRIBUTING.md instructions

5e210a6

Typo

a0ca381

ES6-ify: minor fixes and lefovers after rebasing on top of 1.0.2

54a6118

ES6-ify: upgrade to rollup 0.38 for proper IE8 builds, fix L.SVG.VML

97d8925

Make linter happy.

a47035e

ES6: Fixing typos and sxrew-ups after big rebase

cabf624

IvanSanchez force-pushed the rollup2 branch from addaa88 to cabf624 Compare January 25, 2017 12:38

IvanSanchez added 2 commits January 25, 2017 13:43

Fix symlink for debugging scripts

241c4d9

ES6: Cleanup old build scripts

07d703f

mourner commented Jan 25, 2017

View reviewed changes

IvanSanchez added 2 commits January 25, 2017 17:01

ES6-ify: Update build system to include git rev in L.version

cecbdf5

ES6-ify: re-enable unit tests replacing L.Path with L.Polyline

4c7a68e

IvanSanchez changed the title ~~WIP: ES6 modules & Rollup~~ ES6 modules & Rollup Jan 25, 2017

Export Path

762d315

perliedman approved these changes Jan 26, 2017

View reviewed changes

simon04 reviewed Jan 26, 2017

View reviewed changes

ES6ify: cleanup old banner file

2a8ac28

IvanSanchez and others added 5 commits January 30, 2017 11:05

ES6-ify: whitespace in var declarations

1d70e88

Merge branch 'master' into rollup2

cafc9e8

ES6-ify: Export toTransformation as L.transformation

90f4e09

ES6-ify: cleanup L.transform exports

463f648

ES6-ify: "import Util" in Transformation and SVG.VML

9602abf

IvanSanchez merged commit 703ae02 into master Jan 30, 2017

IvanSanchez removed the in progress label Jan 30, 2017

simon04 mentioned this pull request Feb 13, 2017

Add index.js files to subdirectories/namespace #5329

Merged

perliedman mentioned this pull request Jul 23, 2017

in 1.1.0 all objects are non extendable #5650

Closed

IvanSanchez deleted the rollup2 branch September 27, 2017 10:18

ghybs mentioned this pull request Feb 12, 2018

Fix(TileLayer.WMS & Canvas): get rid of calls to global L #6048

Merged


		import gitRev from 'git-rev-sync'

		// TODO: There should be a way to skip the git branch+rev when doing a production build


		console.log('Bundling and compressing ' + files.length + ' files...');

		var copy = fs.readFileSync('src/copyright.js', 'utf8').replace('{VERSION}', version),


		// geo/projection

		import * as Projection from './geo/projection/Projection';

ES6 modules & Rollup #4989

ES6 modules & Rollup #4989

Conversation

mourner commented Sep 30, 2016 • edited by IvanSanchez Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathancahill commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mourner Oct 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanSanchez commented Oct 20, 2016

mourner commented Oct 21, 2016

IvanSanchez commented Oct 21, 2016

mourner commented Oct 25, 2016

IvanSanchez commented Oct 25, 2016

IvanSanchez commented Dec 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perliedman left a comment

Choose a reason for hiding this comment

simon04 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanSanchez commented Jan 27, 2017

mourner commented Sep 30, 2016 •

edited by IvanSanchez

Loading

mourner Oct 20, 2016 •

edited

Loading