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

Use ES6 class syntax #3408

Merged
merged 15 commits into from
Oct 19, 2016
Merged

Use ES6 class syntax #3408

merged 15 commits into from
Oct 19, 2016

Conversation

mourner
Copy link
Member

@mourner mourner commented Oct 19, 2016

Closes #3400. Some gotchas:

  • ES6 class prototypes are not enumerable (e.g. you can't do util.extend(obj, Foo.prototype) or use for prop in obj to iterate over methods), so all places where we depend on this need to be fixed.
  • In particular, I had to remove the magic util.bindHandlers in favor of explicit binding with util.bindAll.
  • Evented is now a class rather than a mixin.
  • I added super calls to constructors of all classes that have a parent class (e.g. class Map extends Evented). This is enforced by eslint:recommended, and I think this is a good practice, so I decided to fix this instead of turning off the rules.
  • ES6 classes don't support prototype properties, and generally this is considered an antipattern. So e.g. I removed _enabled prototype property in handlers and replaced return this._enabled with return !!this._enabled. Another consequence is util.setOptions won't work (it depends on options prototype object) so we're ditching it.
  • module.exports can't come before class ... — using class before it is defined is a JS error (unlike using function).
  • If you have a circular dependency between a ES6 class and a ES6 class that extends it, you have to require child class after module.exports in the main class, otherwise the child class will try to extend undefined and fail.
  • ES6 class syntax doesn't allow method names with spaces (for worker callbacks like get anchors), but I don't see any reason not to rename those to camelCase.
  • I'm removing @class Foo jsdoc comments because documentation infers that type of thing now.

I suggest reviewing this PR commit by commit, and merging it without squash too, since all commits are self-contained, and the total diff is too big (5k+).

Checklist:

  • Make sure the generated docs are still correct. Current docs are broken in master, so I suggest fixing as a follow-up PR.
  • Run benchmarks.

Progress:

  • data/array_group.js
  • data/bucket.js
  • data/bucket/circle_bucket.js
  • data/bucket/fill_bucket.js
  • data/bucket/fill_extrusion_bucket.js
  • data/bucket/line_bucket.js
  • data/bucket/symbol_bucket.js
  • data/buffer.js
  • data/buffer_group.js
  • data/feature_index.js
  • geo/coordinate.js
  • geo/lng_lat.js
  • geo/lng_lat_bounds.js
  • geo/transform.js
  • render/frame_history.js
  • render/line_atlas.js
  • render/painter.js
  • render/vertex_array_object.js
  • source/geojson_source.js
  • source/geojson_worker_source.js
  • source/geojson_wrapper.js
  • source/image_source.js
  • source/raster_tile_source.js
  • source/source_cache.js
  • source/tile.js
  • source/tile_coord.js
  • source/vector_tile_source.js
  • source/vector_tile_worker_source.js
  • source/video_source.js
  • source/worker.js
  • source/worker_tile.js
  • style/animation_loop.js
  • style/image_sprite.js
  • style/light.js
  • style/style.js
  • style/style_layer.js
  • style/style_layer/background_style_layer.js
  • style/style_layer/circle_style_layer.js
  • style/style_layer/fill_style_layer.js
  • style/style_layer/line_style_layer.js
  • style/style_layer/raster_style_layer.js
  • style/style_layer/symbol_style_layer.js
  • style/style_layer_index.js
  • style/style_transition.js
  • symbol/anchor.js
  • symbol/collision_feature.js
  • symbol/collision_tile.js
  • symbol/glyph_atlas.js
  • symbol/glyph_source.js
  • symbol/sprite_atlas.js
  • ui/camera.js
  • ui/control/attribution_control.js
  • ui/control/control.js
  • ui/control/geolocate_control.js
  • ui/control/navigation_control.js
  • ui/control/scale_control.js
  • ui/handler/box_zoom.js
  • ui/handler/dblclick_zoom.js
  • ui/handler/drag_pan.js
  • ui/handler/drag_rotate.js
  • ui/handler/keyboard.js
  • ui/handler/scroll_zoom.js
  • ui/handler/touch_zoom_rotate.js
  • ui/hash.js
  • ui/map.js
  • ui/marker.js
  • ui/popup.js
  • util/actor.js
  • util/dictionary_coder.js
  • util/dispatcher.js
  • util/evented.js
  • util/lru_cache.js
  • util/struct_array.js
  • util/vectortile_to_geojson.js
  • util/worker_pool.js

сс @jfirebaugh @lucaswoj @mollymerp

Note that `util.bindHandlers` no longer works with ES6 classes
(prototype methods are not enumerable), so I removed it in favor of
more explicit `util.bindAll`.
@mourner mourner force-pushed the es6-classes branch 2 times, most recently from 4b4c641 to 59c871a Compare October 19, 2016 12:13
@mourner
Copy link
Member Author

mourner commented Oct 19, 2016

Updating the top comment with new gotchas as I go. There are quite a few, but all are reasonable.

@mourner
Copy link
Member Author

mourner commented Oct 19, 2016

Benchmarks (freezed on geojson-setdata-large master, so slightly incomplete):

map-load
master 15960ff: 1,345 ms
es6-classes d261612: 184 ms

style-load
master 15960ff: 196 ms
es6-classes d261612: 188 ms

buffer
master 15960ff: 1,193 ms
es6-classes d261612: 1,212 ms

fps
master 15960ff: 60 fps
es6-classes d261612: 59 fps

frame-duration
master 15960ff: 10.2 ms, 15% > 16ms
es6-classes d261612: 10.3 ms, 15% > 16ms

query-point
master 15960ff: 1.07 ms
es6-classes d261612: 1.13 ms

query-box
master 15960ff: 103.44 ms
es6-classes d261612: 106.99 ms

geojson-setdata-small
master 15960ff: 11 ms
es6-classes d261612: 8 ms

@mourner
Copy link
Member Author

mourner commented Oct 19, 2016

This is ready for review. I'm leaving out struct_array.js (will require a non-trivial rewrite because it's currently too complex) and also camera.js mixin (better as a follow-up PR, with discussion on how to compose it better). The docs are not building, but they're broken in master too, so I'll leave fixing docs and making sure they're correct to a follow-up PR too.

@jfirebaugh
Copy link
Contributor

None of the gotchas seem particularly concerning to me.

This will definitely cause merge conflicts. The one PR I'd call out specifically is #3402, because it's on a deadline.

@mourner
Copy link
Member Author

mourner commented Oct 19, 2016

@jfirebaugh yeah, I specifically reviewed that one to make sure that it'll be relatively easy to rebase — I can help with that after this PR is merged.

@jfirebaugh
Copy link
Contributor

Great. I'm 👍 here, but will let @lucaswoj do the review approval and final say on when to time the merge.

Copy link
Contributor

@lucaswoj lucaswoj left a comment

Choose a reason for hiding this comment

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

👍

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

Successfully merging this pull request may close these issues.

Use native ES6 classes
3 participants