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

Update geocoder_address_component.dart #140

Closed
wants to merge 2 commits into from
Closed

Conversation

OlehSv
Copy link

@OlehSv OlehSv commented Sep 3, 2024

Fix for issue #138

@matanshukry
Copy link

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

@SherpaMiguel
Copy link

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

@a14n
Copy link
Owner

a14n commented Sep 27, 2024

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
Copy link
Owner

a14n commented Sep 27, 2024

I didn't see #138 (comment) . 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
Copy link
Contributor

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
Copy link

srujzs commented Sep 27, 2024

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
Copy link
Owner

a14n commented Sep 27, 2024

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
Copy link

srujzs commented Sep 27, 2024

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
Copy link
Owner

a14n commented Sep 27, 2024

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

The fix is now published in google-maps-8.1.0

@a14n a14n closed this Sep 27, 2024
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

Successfully merging this pull request may close these issues.

6 participants