for...in iterating through inherited properties

[gquagliano]

Hello. I've noticed that there are many for...in loops that iterate over all properties, including inherited ones. This leads to errors when some other script has added properties to the prototype. For instance, I ran into this error when I tried to create a map in a page where other script had added an extra property to Object's prototype:

TypeError: Cannot read property 'fn' of undefined at NewClass._on (leaflet-src.js:510)

Adding properties to native prototypes may be a bad practice, but sometimes you don't have control over it. And it's also a good practice to use hasOwnProperty with for...in loops.

Should I push these validations? I added them in my local copy and the app is working fine now.

• I've looked at the documentation to make sure the behavior is documented and expected
• I'm sure this is a Leaflet code issue, not an issue with my own code nor with the framework I'm using (Cordova, Ionic, Angular, React…)
• I've searched through the issues to make sure it's not yet reported

Steps to reproduce
Steps to reproduce the behavior:

• Alter Object prototype (Object.prototype.test="test";)
• Create a new map (L.map())

Expected behavior
Map should be created normally.

Current behavior
Fails due to the unexpected property test in Object.

Environment

• Leaflet version: 1.7.1
• Browser (with version): Opera 73.0
• OS/Platform (with version): Windows 10

Additional context
See intro.

Minimal example reproducing the issue

https://plnkr.co/edit/iLQ7Mlm5tbvZMgyv

• this example is as simple as possible
• this example does not rely on any third party code

[johnd0e]

And it's also a good practice to use hasOwnProperty

Or Object.keys(...).forEach(function(...) { (if we do not care about IE<9).

But I also should mention that there is another good practice - do not extend Object prototype.

[gquagliano]

That's exactly what I said too, but what if you don't have control over it (it's done by other library that you need)? Isn't this a case worth considered? Specially given how simple the solution is. Thank you for your time and reply.

[johnd0e]

Oh, forgot to mention that direct use of hasOwnProperty is not good practice too: preferable form is Object.prototype.hasOwnProperty.call(this, ...).

Specially given how simple the solution is.

It is not that simple, as it requires a hundreds of changes.
And what is also important (as for me): additional wrapping in extra if adds visual complexity to very trivial pieces of code.

what if you don't have control over it (it's done by other library that you need)?

Well, are you sure that it is really unique lib, and you are not able to replace it?
Even if you adapt Leaflet, there may occur some another incompatible dependency (incl. Leaflet's plugin) that is also out of your control.

[gquagliano]

Ok. It's clear, thank you for your insight.