Inconsistent passing of options in L.geoJSON()

[codeofsumit]

• 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:

• Step 1: Create GeoJSON with points and polygons
• Step 2: Add them to a map through L.geoJSON() and pass any options to it
• Step 3: look at the options from the layer, or in an initHook. Layers inheriting from L.Path have the options, Markers don't.

Expected behavior
Passing options to L.geoJSON() should pass those options to all child elements. This is especially important for plugin authors that use initHooks on leaflet classes.

Current behavior
Take a look at the console in this JSFiddle to see that the passed option testOption: true is not present for Markers:
https://jsfiddle.net/naw79csr/

Environment

• Leaflet version: 1.5.1
• Browser (with version): Chrome 77
• OS/Platform (with version): MacOS Mojave 10.14.6

Additional context
We found the problem through a bug report in Leaflet-Geoman geoman-io/leaflet-geoman#508

As a test, I've passed the options parameter in the leaflet source to new L.Marker here

Leaflet/src/layer/GeoJSON.js

Line 184 in 16c49cc

return pointToLayer ? pointToLayer(geojson, latlng) : new Marker(latlng);

and here

Leaflet/src/layer/GeoJSON.js

Line 189 in 16c49cc

layers.push(pointToLayer ? pointToLayer(geojson, latlng) : new Marker(latlng));

and it fixed the issue.

Minimal example reproducing the issue
https://jsfiddle.net/naw79csr/

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

As always, thank you for leaflet and your outstanding work 👏

[ghybs]

Hi and thank you for the detailed bug report!

Hum I can see the use case and simple fix indeed.

So the idea is to mimick the behaviour of "custom" options of path/vector layers for point type features, namely for the default created Marker.

Now I am concerned about the breaking change possibly affecting existing apps.
Markers from GeoJSON factory will be affected by options which were previously ignored:

L.geoJSON(pointFeature, {
  opacity: 0.5 // Initially intended for path layers stroke opacity
});

Currently the created Marker is standard.
With the proposed change, it will have a different opacity:
https://plnkr.co/edit/KwgtdognmorABgETUjko?p=preview

Then there is also a "hole" left for pointToLayer: custom layer would not automatically inherit the custom options.

[codeofsumit]

@ghybs regarding pointToLayer:

Yes, the current workaround is to just use pointToLayer and pass the options there. So I think it's fine that in the case of a "manual" creating of the layer, the options need to be passed "manually" as well.

But your mentioned breaking change is definitely a problem I haven't seen yet 🤔. Although in your example, as a user, I would expect the Markers have .5 opacity and consider the fact that it's ignored a bug. Are you aware that devs actually rely on that as a feature? I have not seen this mentioned in the documentation.

If this was an intentioned feature: it would be a breaking change
If not: it's a bug fix => patch

But I can see there is room for interpretation here 😅 .

Do you think an (ugly) additional option like passToMarkers: true that would make the options passed to markers and defaults to false would be a way to add this to a minor/patch version bump (and removing the option in the next major release)?

[ghybs]

Hi,

Yeah unfortunately there is a lot of room for interpretation (classic "it is not a bug, it is a (undocumented) feature")...

To stay safe, and even though it may look ugly at first, I think your proposed passToMarkers option solution would be appropriate enough for such situation.

Let's do this.