Upgrade babel to 7+; prefer class properties

mapbox / mapbox-gl-accessibility

An accessibility control for Mapbox GL JS

https://labs.mapbox.com/mapbox-gl-accessibility

ISC License

68 stars 21 forks source link

Upgrade babel to 7+; prefer class properties #34

Closed allthesignals closed 4 years ago

allthesignals commented 4 years ago

I ran into some snags trying to get a testing environment setup in a way that mimics other mapbox plugins (mapbox-gl-draw).

This change does a few things:

Updates all babel package naming and features to Babel 7+. Specifically, this moves to @babel/preset-env, which tries to auto-detect needed transpilations.
Swaps the old transform-class-properties package naming to the new @babel style.
Removes babel-cli
Reverts 04a9bf67831f3eaadb63eab22ebf2291cea1b197 by switching back to class property proposal syntax, which implicitly handles method binding.

Instead of a workaround, this PR addresses the original issue reported in #17.

andrewharvey commented 4 years ago

thanks for working on this @allthesignals. High level comments from me:

mapbox-gl-draw and many other libraries have switched to using rollup (instead of browserify), do you think rollup would be better?
if we're now building a dist js file, we should output that to the dist directory rather than the example directory

allthesignals commented 4 years ago

mapbox-gl-draw and many other libraries have switched to using rollup (instead of browserify), do you think rollup would be better?

if we're now building a dist js file, we should output that to the dist directory rather than the example directory

Agree! I trust whatever is standard among other plugins.

I'll be happy to make those changes. Would you like to see me make those changes in this PR or a follow-up PR?

allthesignals commented 4 years ago

@andrewharvey I went ahead and switched over to rollup. It now builds into the dist folder. There are some other clean-up tasks:

Update the start script so it uses the same stack as mapbox-gl-draw
Switch to yarn over npm

allthesignals commented 4 years ago

Hi @tristen @andrewharvey can i get some feedback on this?

andrewharvey commented 4 years ago

I have no objections with these changes, though admittedly I don't have much experience with js build systems. I'll see if @tristen has an opinion first, if not then I can approve and merge.

tristen commented 4 years ago

Apologies for my tardiness. No objections from me either 👍