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

L.Util L.DomUtil is not extensible in leaflet 1.1.0? #5589

Closed
rendrom opened this Issue Jun 28, 2017 · 8 comments

Comments

Projects
None yet
7 participants
@rendrom

rendrom commented Jun 28, 2017

Сan not add a custom property to an L.Util or L.DomUtil object since leaflet version 1.1.0:
https://jsfiddle.net/rendrom/Lj63c72u/

But it works in 1.0.3 and earlier:
https://jsfiddle.net/rendrom/scvpb4s2/

This is a new behavior or ES6 Rollup build result?

Example incompatibility in plugins:
https://github.com/w8r/Leaflet.Modal/blob/master/src/modal.js#L16

@IvanSanchez

This comment has been minimized.

Show comment
Hide comment
@IvanSanchez

IvanSanchez Jun 28, 2017

Member

This is a byproduct of the Rollup build. If you look at the built files, you'll find:

var Util = (Object.freeze || Object)({
	extend: extend,
	create: create,
	bind: bind,
        // etc

This happens because if you code something like this in ES2015...

import Util from 'leaflet/Util';

Util.somethingNew = 'foo'; // Error here, can not add things to an imported module

You can check this by visiting this link to the Rollup REPL

The current situation is that plugins are able to modify Leaflet's namespaces (I'm guilty as charged, as I'm defining L.Browser.gl in one of my plugins). The problem is that another plugin could come, reimplementing that very same namespace, and making the previous plugin fail.

The ""right"" approach would be to either:

  • Put that functionality into the plugin's namespace, without touching the L namespace
  • If that functionality is to be used elsewhere, make it its own module, and import it accordingly.
Member

IvanSanchez commented Jun 28, 2017

This is a byproduct of the Rollup build. If you look at the built files, you'll find:

var Util = (Object.freeze || Object)({
	extend: extend,
	create: create,
	bind: bind,
        // etc

This happens because if you code something like this in ES2015...

import Util from 'leaflet/Util';

Util.somethingNew = 'foo'; // Error here, can not add things to an imported module

You can check this by visiting this link to the Rollup REPL

The current situation is that plugins are able to modify Leaflet's namespaces (I'm guilty as charged, as I'm defining L.Browser.gl in one of my plugins). The problem is that another plugin could come, reimplementing that very same namespace, and making the previous plugin fail.

The ""right"" approach would be to either:

  • Put that functionality into the plugin's namespace, without touching the L namespace
  • If that functionality is to be used elsewhere, make it its own module, and import it accordingly.
@MuellerMatthew

This comment has been minimized.

Show comment
Hide comment
@MuellerMatthew

MuellerMatthew Jul 6, 2017

Contributor

Since this is a breaking issue with the previous versions of Leaflet, would it make sense to rollback the change temporarily, and issue a console warning or an alert of some sort saying that the ability is depreciated and that it will be removed in the next version? There appears to be numerous published and unpublished plugins which used this feature.

Contributor

MuellerMatthew commented Jul 6, 2017

Since this is a breaking issue with the previous versions of Leaflet, would it make sense to rollback the change temporarily, and issue a console warning or an alert of some sort saying that the ability is depreciated and that it will be removed in the next version? There appears to be numerous published and unpublished plugins which used this feature.

thorinii added a commit to thorinii/Leaflet.FileLayer that referenced this issue Jul 7, 2017

Fix to work with Leaflet 1.1
Leaflet 1.1.0 switched to using Rollup as their bundler tool. It broke
a number of plugins that depended on being able to install things in
namespaces like `L.Util`. Leaflet.FileLayer was affected because it
installs `L.Util.FileLoader` and `L.Util.fileLoader`.

This commit moves those into a new namespace: `L.FileLayer`.

It also changes the tests to adjust to where Rollup puts the Leaflet
distribution.

See Leaflet/Leaflet#5589.
@perliedman

This comment has been minimized.

Show comment
Hide comment
@perliedman

perliedman Jul 7, 2017

Member

@MuellerMatthew in hindsight, it might have been a bad idea to introduce this change without bumping the major version, as you say this is a breaking change, or at least breaks a pretty common pattern, even though I don't think it has been explicitly documented or supported.

Unfortunately, we are way beyond reverting the ES6 change, it touches an extremely large portion of the source and backing it out would also undo almost all changes we made since 1.0.3.

I think the only way forward is to clearly state that plugins can/should only add methods to real classes, not namespaces.

Member

perliedman commented Jul 7, 2017

@MuellerMatthew in hindsight, it might have been a bad idea to introduce this change without bumping the major version, as you say this is a breaking change, or at least breaks a pretty common pattern, even though I don't think it has been explicitly documented or supported.

Unfortunately, we are way beyond reverting the ES6 change, it touches an extremely large portion of the source and backing it out would also undo almost all changes we made since 1.0.3.

I think the only way forward is to clearly state that plugins can/should only add methods to real classes, not namespaces.

@MuellerMatthew

This comment has been minimized.

Show comment
Hide comment
@MuellerMatthew

MuellerMatthew Jul 7, 2017

Contributor

Thats fine. I think we should probably updated the documentation for plugins then so that we say that for plugins after 1.0.3 there are different rules.

Under the Plugin API title it currently says:

If you have a new class, put it directly in the L namespace (L.MyPlugin).
If you inherit one of the existing classes, make it a sub-property (L.TileLayer.Banana).
Every class should have a factory function in camelCase, e.g. (L.tileLayer.banana).
If you want to add new methods to existing Leaflet classes, you can do it like this: L.Marker.include({myPlugin: …}).

https://github.com/Leaflet/Leaflet/blob/master/PLUGIN-GUIDE.md

Contributor

MuellerMatthew commented Jul 7, 2017

Thats fine. I think we should probably updated the documentation for plugins then so that we say that for plugins after 1.0.3 there are different rules.

Under the Plugin API title it currently says:

If you have a new class, put it directly in the L namespace (L.MyPlugin).
If you inherit one of the existing classes, make it a sub-property (L.TileLayer.Banana).
Every class should have a factory function in camelCase, e.g. (L.tileLayer.banana).
If you want to add new methods to existing Leaflet classes, you can do it like this: L.Marker.include({myPlugin: …}).

https://github.com/Leaflet/Leaflet/blob/master/PLUGIN-GUIDE.md

@oberhamsi

This comment has been minimized.

Show comment
Hide comment
@oberhamsi

oberhamsi Jul 18, 2017

I would suggest also putting this into the changelog.i guess there are lots of plugins extending L.Util - i found utgrid and filelayer do that.

oberhamsi commented Jul 18, 2017

I would suggest also putting this into the changelog.i guess there are lots of plugins extending L.Util - i found utgrid and filelayer do that.

@calvinmetcalf

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Jul 18, 2017

Contributor

so fyi the issue is actually import * as foo from 'bar' vs import foo from 'bar'

Contributor

calvinmetcalf commented Jul 18, 2017

so fyi the issue is actually import * as foo from 'bar' vs import foo from 'bar'

@calvinmetcalf

This comment has been minimized.

Show comment
Hide comment
@calvinmetcalf

calvinmetcalf Jul 20, 2017

Contributor

actually it's slightly worse as L is never exported as an object you can't add anything to L either

Contributor

calvinmetcalf commented Jul 20, 2017

actually it's slightly worse as L is never exported as an object you can't add anything to L either

@mourner

This comment has been minimized.

Show comment
Hide comment
@mourner

mourner Jul 24, 2017

Member

Merging this issue with #5650.

Member

mourner commented Jul 24, 2017

Merging this issue with #5650.

@mourner mourner closed this Jul 24, 2017

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