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

Overrides break method chaining of leaflet #29

Open
codeofsumit opened this issue Feb 20, 2019 · 9 comments
Open

Overrides break method chaining of leaflet #29

codeofsumit opened this issue Feb 20, 2019 · 9 comments

Comments

@codeofsumit
Copy link
Contributor

codeofsumit commented Feb 20, 2019

Hey Per, me again :-)

Got an issue reported in leaflet.pm regarding compatibility. It's the same problem as last time, the methods here are overriding leaflet methods but do not return this:

setLatLng: override(L.Circle.prototype.setLatLng, function() {

Hence, chaining leaflet methods like layer.setLatLng().redraw() throw an error. From a quick look I see the problem in all of these methods: onRemove, setLatLng, setRadius.

In the leaflet source, these function return the event call fire which is returning the instance. When overriding these function, the same instance should be returned.

Should be fixed by adding return this to each function. Can you quickly add that fix or should I create a PR?

Wish you all the best 👍
Sumit

@perliedman
Copy link
Contributor

Hi Sumit 👋 !

Thanks for the report, I think I fixed this now and released version 1.4.2 with the fix!

@Jumpy555
Copy link

Hi,
thank you both for your work.
I tried the last version but the problem with chaining methods still persists.

When used with the previous version of leaflet-measure-path, leaflet.pm returned this error:
TypeError: this._layer.setLatLng(...) is undefined

Now, with the last version of leaflet-measure-path, leaflet.pm returns this error:
TypeError: this._layer.setLatLng(...).redraw is not a function

Splitting the methods setLatLng and redraw in leaflet.pm resolves the problem.

@codeofsumit
Copy link
Contributor Author

I will split it in the next release to avoid the problem. Nevertheless, chaining is a leaflet functionality that should not be broken. I’m on a phone right now so I can’t Check leaflet-measure-Path but @perliedman if you need help debugging let me know and I will have a look this weekend.

@perliedman
Copy link
Contributor

Hm, let's see if I can add some tests for this to make sure it actually works as intended, I agree chaining should work.

@perliedman perliedman reopened this Feb 21, 2019
@perliedman
Copy link
Contributor

Made a pretty obvious mistake when "fixing" the problem this morning 7e554e9 😖

Will add tests when I find the time to make sure this doesn't happen again.

@codeofsumit
Copy link
Contributor Author

@Jumpy555 can you test again?

@Jumpy555
Copy link

@codeofsumit the chaining problem is still present

@RobinBressan
Copy link

RobinBressan commented Mar 9, 2019

@perliedman any news on this issue? A fix was done on LeafletPM to handle it, but using leaflet-measure-path with leaflet pm still gives me weird behavior on dragging, and I assume it's still linked to this issue, which might prevent PM from doing its work.

@perliedman
Copy link
Contributor

Sorry, not enough time to look close at this. In general, I have very limited time for open source at the moment.

I expect this to be some minor detail, so any help from someone actually suffering from this issue would be greatly appreciated!

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

No branches or pull requests

4 participants