a14n / dart-google-maps

A library to use Google Maps JavaScript API v3 from Dart scripts.
Apache License 2.0
130 stars 66 forks source link

Update geocoder_address_component.dart #140

Closed OlehSv closed 2 months ago

OlehSv commented 2 months ago

Fix for issue #138

matanshukry commented 2 months ago

@a14n Can you take a look and hopefully merge it and publish the new version? My plugin depends and waits for this fix

SherpaMiguel commented 2 months ago

Commits look nice. It would be great to merge it. Many plugins are dependant of this.

a14n commented 2 months ago

Sorry for the delay.

All files in the /generated/ folder shouldn't be touch manually. I will try to take a look at this but I don't have an account allowing me to test geocoding parts.

That said, from a technical point of view dartify should recursively converts the JS object to a Dart object . So it looks like a bug in JS interop because _types.toDart.map((type) => type.toDart).toList(); works and _types.dartify() as List<String> fails. GeocoderAddressComponent.types is documented to be a array<String>. @srujzs @sigmundch any idea?

a14n commented 2 months ago

I didn't see https://github.com/a14n/dart-google-maps/issues/138#issuecomment-2295845120 . So the error suggest that dartify returns a List<String?> and we should cast it to a list of non-nullable elements. I will try to land a fix quickly.

sigmundch commented 2 months ago

So the error suggest that dartify returns a List<String?>

I believe you are correct that this is likely the cause of the error. I believe the implementation will return a List<Object?>.

Re using dartify vs .toDart.map(=> .toDart):

It may not be a big concern if this code is not in a critical path or if the list is overall small, but note that the latter is a lot more efficient. dartify needs to check for multiple kinds of types on each value to recognize what to do. So the cost depends a lot in what order it checks for things. Looking at the implementation, I see arrays are far down the list, while string are close to the top. I estimate about 21 branch conditions upfront to detect that the top value is an array, and then about 4 conditions on every list element.

srujzs commented 2 months ago

I also doubt we can do better with dartify in terms of typing without giving up some performance. All data within the array can be any Dart type when converted, so we're forced to use Object?. We can maybe imagine keeping track of array contents to detect if there are multiple types, but we'd need to pass over the contents twice, either to move the List<Object?> to a List<String> or first check the array contents. Maybe an arg can be passed asking us to assume there's only one type always in every container, or we can introduce a cast list after we verified it's only one type. Maybe with the former, this would allow us to avoid further type-checks after the first element.

a14n commented 2 months ago

Thanks @sigmundch and @srujzs for your advice! Out of curiosity, had you evaluate to make dartify generic to allow user to provide an hint of what to return jsObject.dartify<List<String>>() ?

srujzs commented 2 months ago

I definitely want to make jsify/dartify generic so that users don't have to manually cast (among other improvements/revisions to those APIs), but that won't really address the use case here. Dissecting the given type is a cool thought - in this case, extracting the String from List<String> - but I don't believe the language allows you to do something like that. Assuming the provided type can be resolved statically, we could maybe use a transform of some kind which would allow us to do that, but the number of edge cases there gives me pause.

a14n commented 2 months ago

@OlehSv thanks for your PR. I will close it because I regenerated the lib (in 80e6e601604ad96bbc74de77740bdb37d3c98e88) with this fix to handle all members returning a List<String>.

The fix is now published in google-maps-8.1.0