Raruto / leaflet-elevation

Leaflet plugin that allows to add elevation profiles using d3js
GNU General Public License v3.0
254 stars 82 forks source link

Replace native `structuredClone` with custom `cloneDeep` function #256

Closed Raruto closed 1 year ago

Raruto commented 1 year ago

Fixes a regression introduced by: https://github.com/Raruto/leaflet-elevation/pull/240

Closes: https://github.com/Raruto/leaflet-elevation/issues/255

Raruto commented 1 year ago

@hupe13 let me know.

hupe13 commented 1 year ago

The error is gone, but I don't see both on my test page and on your example the marker grafik.

Raruto commented 1 year ago

Check this out now and also check out the other examples with this change because it's quite likely that something else could be broken..

hupe13 commented 1 year ago

I went through my test pages and didn't find any errors because of this, but I found some errors in my application. Your example works, but there is a warning that leaflet-rotate is missing. What should I look for that might be broken?

Raruto commented 1 year ago

there is a warning that leaflet-rotate is missing.

https://github.com/Raruto/leaflet-elevation/blob/868179234d8e9b8f7beaea57d0e34d8bd79a2622/libs/leaflet-distance-marker.js#L135-L141

What should I look for that might be broken?

Vanilla JS and Leaflet JS are not to able clone (or merge) complex data structures (eg. nested objects and instances of classes), that's why we needed to add a deep clone algorithm for the default options (eg: to fix https://github.com/Raruto/leaflet-elevation/issues/238#issuecomment-1493826767).

// GOAL: merge "foo.bar.a" and "foo.bar.b" attributes --> { foo: { bar: { a: 'some value', b: 'another value'} } }

const foo_bar_a = { foo: { bar: { a: 'some value'} } };
const foo_bar_b = { foo: { bar: { b: 'another value'} } };

const leaflet_result = L.extend({}, foo_bar_a, foo_bar_b);
const native_result  = Object.assign({}, foo_bar_a, foo_bar_b);

console.log(leaflet_result.foo.bar); // LOGS: { b: 'another value'}
console.log(native_result.foo.bar); // LOGS: { b: 'another value'}

So, taking a quick look, these are some options that I think might break:

https://github.com/Raruto/leaflet-elevation/blob/b0006d635a048ec3d1db519819ad3a9813e7ffc6/src/options.js#L24-L29

https://github.com/Raruto/leaflet-elevation/blob/b0006d635a048ec3d1db519819ad3a9813e7ffc6/src/options.js#L56-L63

https://github.com/Raruto/leaflet-elevation/blob/b0006d635a048ec3d1db519819ad3a9813e7ffc6/src/options.js#L73

https://github.com/Raruto/leaflet-elevation/blob/b0006d635a048ec3d1db519819ad3a9813e7ffc6/examples/leaflet-elevation_extended-ui.html#L255-L271

I went through my test pages and didn't find any errors because of this, but I found some errors in my application.

I don't think I understand what you mean..

hupe13 commented 1 year ago

I went through my test pages and didn't find any errors because of this, but I found some errors in my application.

I don't think I understand what you mean..

I found some errors in my code. (My english is not good, I'm using translators.)

Raruto commented 1 year ago

Ok, so I merge this pull request and then I release a new version so you can easily test all the latest changes together.

Ref: #252, #253, #254, #256 and #228