Upgrade LeafletJS to 1.x

[bhaskarvk]

The issue

Now that LeafletJS 1.x has been out and stable for a while, it's time to think about upgrading to it. However it's not as simple and as straight forward as it sounds, so I'm creating this issue to capture all information related to the upgrade.

Initial Assessment

The first order of thing is to adapt any existing code to use 1.x API, and the good news is that not much has changed, so most of the code can work as is. The only two things I am aware of are

• L.circle now accepts radius as an option (like L.circleMarker) rather than a second argument
• Labels are not supported by the Leaflet.label plugin anymore, rather they are incorporated in the core dist. and renamed to L.Tooltip

These are both minor changes can can be done only on the JS side w/o having to rewrite the R API.

Now for the hard parts.

The hardest part is going to be upgrading the plugins used. As plugins are individual contributions, it is up to the plugin author to migrate to 1.x and unfortunately not all have done so. We need to review this on a case by case basis. Starting with Proj4Leaflet which is critical in supporting custom projections. Proj4Leaflet hasn't been fully migrated to 1.x, but a branch does exists claiming to support 1.x. The problem is that the maintainer of Proj4Leaflet has not found spare time to work on it in a while and the branch is > 6 months old.

Next are some other plugins we use in the package such as AwesomeMarkers, Measure, Minimap, Terminator, omnivore, leaflet-providers, Easybutton, marker cluster, location filter (for crosstalk).
The repos of all these plugins needs to be examined and we need to determine what efforts will require for each of them.

Lastly we have the leaflet sup-packages, starting with leaflet.extras (by your's truly), and mapview. These two use a lot of leaflet plugins and they too must be examined for the level of efforts required.

The benefits

• 0.7 branch is no longer being developed or fixed.
• 1.x branch has a lot of performance and bug fixes.
• 1.x has support for some really cool stuff like vector tiles, webgl and more.
• Always pays off to stay current.

Action Plan

• I can take a first stab at porting just the leaflet package (along with any plugins it uses) to 1.x
• For preventing any compatibility issues I will call this package leaflet2 (leaflet2 which is based on LeafletJS 1.x, I know how ironic that sounds).
• Once this is in a usable condition, I'll begin to work on leaflet.extras and more.

@jcheng5 @hadley @timelyportfolio @tim-salabim @mdsumner Please provide your inputs on this. This is quite critical in keeping the package up to date and viable for the coming years.

One thing you guys can help out immediately is discussing whether this needs to be a new package (leaflet2), or simply continue with the same name. I don't think compatibility is going to be an issue for users of just this packge, but for sub-package it could lead to problems if we retain the same name. e.g. some plugins in mapview / leaflet.extras may not have been ported to 1.x, and ergo won't work with the latest leaflet package (if we were to retain the same name).

[tim-salabim]

This came around on R-pkg-devel today

https://mailman.stat.ethz.ch/pipermail/r-package-devel/2017q1/001256.html

Quote @eddelbuettel :

"AFAIK that practice of package inflation is outlawed by CRAN Policy now:

 * Changes to CRAN packages causing significant disruption to other
   packages must be agreed with the CRAN maintainers well in advance of
   any publicity. Introduction of packages providing back-compatibility
   versions of already available packages is not allowed.

R itself has mechanisms for this: .Deprecated was already mentioned.

Many other packages "warn now", phase in new code and then offer a toggle
(maybe via options()) to get the old behaviour.

As package authors, we have a "contract" with our users. Sure, we can break
it willy-nilly (and the informal reputation mechanism may make us pay for
this) or we can try to hold the contract and accomodate.

It's programming. There is always another layer we can insert."

[jcheng5]

Let's not decide on any solutions until we know how bad the breakage is. I'm going to spend some time on Leaflet starting next week, and can start with Leaflet 1.0 testing.

I don't feel comfortable though releasing 1.1 of this package to CRAN until we at least have some confidence that we're not going to have to drop any of the recently introduced features (I don't think it's likely, but better to know).

[bhaskarvk]

FWIW I do think that eventually everything will work with 1.x and the situation is not terrible. I agree with your concerns regarding pushing the new stuff in the package to CRAN.

Would you have a few mins to discuss this over a call?

[bhaskarvk]

Woohoo, Proj4Leaflet just published 1.0 support kartena/Proj4Leaflet#126

[timelyportfolio]

I'd like to help in this effort. Should we create a project https://github.com/rstudio/leaflet/projects under this Github repo to track progress?

[bhaskarvk]

I have already started a bit of work on 1.0. But would love to cooperate. @timelyportfolio For leaflet we're already using waffle.io https://waffle.io/rstudio/leaflet.
Personally I like waffle coz you can track multiple repos as opposed to GH projects which I tied to the repo.

[timelyportfolio]

Do we have a working dev branch with > 1.0? I'd love to start tackling remaining items especially regarding the core R leaflet API.

[bhaskarvk]

Actually I am swamped with personal stuff + work stuff. Don't let me hold you back.

[timelyportfolio]

So, anybody out there listening, just to confirm no one else is working or has worked on this problem? I just want to make sure that in a world of scarce resources that I am not duplicating efforts. Thanks!

[jcheng5]

I'm not working on it either yet.

[tim-salabim]

Is there an expected timeline? And how detrimental will this update likely be for existing leaflet functionality?

[timelyportfolio]

wish me luck :) https://github.com/timelyportfolio/leaflet/tree/v1.0

[timelyportfolio]

Don't expect this to be easy, but R and JS tests all pass. Now, for the hard slog...

[bhaskarvk]

I won't be too encouraged with the tests passing. They are fairly rudimentary ;)

From my notes here're a couple of things to watch out for.

• L.circle now accepts radius as an option (like L.circleMarker) rather than a second argument
• Labels are now Tooltips and part of core JS lib, but no need to change R API. Just delete the leaflet-label JS plugin and change all L.label to L.tooltip.

Some big plugins you'll need to watch out for

• Marker Cluster
• Proj4Leaflet (This one got 1.x compatibility only recently)

[timelyportfolio]

Thanks for the headstart. Hopefully I can add some more tests that will help in future since the cadence for leaflet is now much quicker. I will try to log all I find in https://github.com/timelyportfolio/leaflet/blob/v1.0/inst/errors/errors.Rmd.

[bhaskarvk]

@timelyportfolio One more note. Don't edit inst/htmlwidgets/leaflet.js directly. It's auto generated.
Change code in src/javascript/* and run grunt build. See README.md for details.
Also code in src/javascript is ECMA6 which is transpiled by grunt into ECMA5.

[timelyportfolio]

@jcheng5, I noticed that many of the initial control plugins do not remove properly (not leaflet >1.0 problem since doesn't work currently). I then discovered the "new" ControlStore that seems to be built for these controls. However, I couldn't find anything that uses the new ControlStore. The API for it seems straightforward, but are there any live controls that can demonstrate coding style, best-practice that I can use to fix the old controls? Thanks!

[bhaskarvk]

@timelyportfolio Sample code for the issue?

[timelyportfolio]

@bhaskarvk, thought you were busy :)

Failing Controls

These controls work "fine", but we had added code to insure that controls are not added twice.

layers lines

leaflet-measure lines

Here is the R code that demonstrates the error. Note, that in general we do not expect users to intentionally add two of the same controls, but often in a pipe or in composing a leaflet map, this might accidentally happen.

leaflet() %>%
  addTiles(group = "OpenStreetMap") %>%
  addProviderTiles("Stamen.Toner", group = "Toner by Stamen") %>%
  addMarkers(runif(20, -75, -74), runif(20, 41, 42), group = "Markers") %>%
  addLayersControl(
    baseGroups = c("OpenStreetMap", "Toner by Stamen"),
    overlayGroups = c("Markers")
  ) %>%
  addLayersControl(
    baseGroups = c("OpenStreetMap", "Toner by Stamen"),
    overlayGroups = c("Markers")
  )

seems to work, but if we inspect the JavaScript console, we see

Broader Problem

We could solve this by notifying on the R side that there is a duplicate control and choosing by default first or last instance of the control. However, as we progress on a more robust leaflet, I think reworking all controls to have easy, reliable add/replace mechanisms in both R and JavaScript will be very beneficial. I see @jcheng5 ControlStore as the way forward. However, I couldn't find anything that actually uses ControlStore, so I wanted to make sure that I was not missing something before proceeding.

[bhaskarvk]

Bugs always bug me :)

If you look at https://github.com/rstudio/leaflet/blob/master/javascript/src/methods.js#L668, You will notice that addControl will add the control via the ControlStore (this.controls is an instance of ControlStore).
Almost all controls are added via addControl which in turn uses ControlStore.
So if this bug is happening we need to fix it.

[bhaskarvk]

Also from your working example, I think it's a mistake to add layersControl and measure-control directly to the map bypassing ControlStore.
FWIW I think ControlStore should become the de-facto way of adding/managing/removing any control. And any existing code that is not doing so should be changed as part of your refactoring.

[timelyportfolio]

I think all controls are being added/removed in this way, since they predate ControlStore, but I have not done much sleuthing into the chronology. I agree with you that ControlStore is the best mechanism, but I was a little surprised to find no complete examples of adding/managing/removing controls with ControlStore. This could easily be the result of copy/paste :)

[bhaskarvk]

ControlStore is not new, it's been there for a while. What is new is some time mid last year @jcheng5 broke up the monolithic leaflet.js into smaller pieces and used grunt to assemble the final JS.

[bhaskarvk]

Also all what I said about ControlStore, also holds valid for LayerManager. No layers should be directly added to the map.

[timelyportfolio]

ignore what I previously wrote, sorry

[timelyportfolio]

Ok, @bhaskarvk and @jcheng5, I think I have updated leaflet and all plugins to their most recent versions in https://github.com/timelyportfolio/leaflet/tree/v1.0. I tested with all examples in documentation and inst/examples, and I believe I have resolved all issues (very few) that I found. If you have some time, I would very much appreciate if you could test locally.

devtools::install_github("timelyportfolio/leaflet@v1.0")

I'll put out a call on social media for testers once I hear back from you.

[timelyportfolio]

cc @tim-salabim, @walkerke, @mdsumner

[bhaskarvk]

I just glanced at the code changes and looks good to me. Of course I'll need to change leaflet.extras too. But this is a really good start.
Btw there are bunch of PRs from @cpsievert for crosstalk part, and my small PR #400.
So @jcheng5 has his work cut out :)

But really good job.

[jcheng5]

Thanks @timelyportfolio!

[tim-salabim]

From a first quick tests, I see that raster images are not shown anymore

leaflet() %>% addTiles() %>% addRasterImage(mapview::poppendorf[[1]])

Other than that pretty much all the code from my mapview tutorial given yesterday still works (even sync).

[timelyportfolio]

thanks everyone for testing so quickly. I'll debug @tim-salabim raster issue.

[timelyportfolio]

@tim-salabim addRasterImage is broken because L.tileLayer.canvas has been replaced (see lines). I am working on a fix now.

[timelyportfolio]

well addRasterImage now shows something, but unfortunately that something is not right 96667e3

[timelyportfolio]

Issue mentioned above now resolved 96667e3 and 83bee11, but left with two lingering items:

1. raster layer hidden by tile layer - should be easy fix

2. when used with layerControl, after toggling raster layer, tiles not redrawn - not sure level of difficulty here

3. will see pixel error discussed in leafletjs issue Pluggable TileLayer canvas renderer Leaflet/Leaflet#4844

example

[bhaskarvk]

@timelyportfolio Any more updates on this ? Anything I can help out with ?

[timelyportfolio]

With the raster issue #357 (comment) in particular, or all Leaflet > 1.*? I have not circled back to fix the remaining raster problems discussed.

In terms of Leaflet > 1.* , should we create a centralized branch to work through and test while still merging existing/recent pull requests?

[bhaskarvk]

@timelyportfolio I might take a stab at this remaining issue sometime next week. Will let you know how it goes.

[bhaskarvk]

@timelyportfolio Leaflet 1.1.0 released. Unfortunately no time to work on this before UserR.

[bhaskarvk]

Picking up where @timelyportfolio left off. Firstly 1.1.0 is a no go as far as tile loading. I just put 1.1.0 js/css and the tiles are not loading , which sucks coz for the time being we're stuck w/ 1.0.3 unless I figure out what's wrong. For now looking at the raster issue.

[timelyportfolio]

let me know how I can help. don't remember much but I think just needed to bring to front or change z-index.

[bhaskarvk]

1.1.0 is supposed to have fixed that., but for now I am having trouble getting tiles to showup properly w/ it

[bhaskarvk]

1.1.0 is supposed to have fixed that., but for now I am having trouble getting tiles to showup properly w/ it

[jcheng5]

I believe I was able to find and fix the raster issues, at least the ones where tiles appear over the raster and the raster not being redrawn. example

I also don't seem to find any problem with upgrading Leaflet to 1.2.0, but I didn't commit any changes other than the minimum needed to fix the rasters.

I've created a PR for v1.x here: #453

There are a couple of remaining work items that I can think of, I'll add them to the PR description. This thread has gotten a little unwieldy, maybe we can take the discussion over there.

[bhaskarvk]

Awesome news. Although I would recommend that you keep this for now as Github only and allow me some time to also migrate leaflet.extras to 1.x set of plugins. Because of the scope of extras package I'll have to go thru each and every plugin to make sure that it works with 1.x so I will need some time to make it 1.x ready.

[jcheng5]

@bhaskarvk It's not possible for these plugins to be both 0.7 and 1.x ready, right? They will be compatible with either one or the other? If that's the case I think we might need to talk about how we're going to roll all this out.

[bhaskarvk]

It's on a case by case basis. Some plugins support both leaflet versions simultaneously, while others have different releases targeting diff. leaflet versions. Some have been abandoned a while ago and may need fixing to work in leaflet 1.x.
Hence my request for some time to comb thru each plugin one-by-one.

[schloerke]

Fixed in #453