fable-compiler / fable-react

Fable bindings and helpers for React and React Native
MIT License
275 stars 66 forks source link

Using generated GoogleMaps types from ts2fable. #102

Closed nojaf closed 6 years ago

nojaf commented 6 years ago

So there is some good news and some bad.

The good is obviously that all types are the real thing. Types can be accessed by open Google.Maps. To create an instance you go like let pos:LatLng = Google.maps.LatLng.Create(loc.Lat, loc.Lng). Haven't updated readme because well still WIP.

The bad news is that everything is based on the global google.maps namespace. This leads to problems when creating f.ex. a LatLng to pass down as MapProperties.Center for example. Fable will replace Google.maps.LatLng.Create with new window.google.maps.LatLng(). But that can only be access once GM is loaded. Because currently the react map component is wrapper in that withScript HOC, you can't be sure that google.maps exists.

My solution for this, is to load google maps in a script tag in the head. So that once my fable app will load, I'm sure that the namespace will be there. I personally don't have any problems with this approach but others might. I currently just don't see any other way.

nojaf commented 6 years ago

Also, I'm not sure about the naming of the GoogleMaps.fs file and namespace/module. Maybe @MangelMaxime can give a suggestion here.

MangelMaxime commented 6 years ago

Please don't merge this PR.

The google map file needs to go under fable-importrepo.

MangelMaxime commented 6 years ago

But that can only be access once GM is loaded.

If you have a delay between when you add the map and when you can actually use it. Then you should add a props OnMounted (or something like that) that will be trigger when the component is ready to be used.

This is what we are doing in the repl:

As you can see, we are passing the editor instance and monaco module as an argument to the props so users can access it and manipulate it here if need. In the case of google map you probably need to set it into the global scope the withScript dones't do it for you.

nojaf commented 6 years ago

Hmm, I'm unsure how to hook a function in when google maps did indeed load. It happens internally so without changing the original source I don't see how it is possible.

forki commented 6 years ago

I need to continue to create positions before the map is loaded. And loading the map before the bundle doesn't work because it's dead slow.

Florian Verdonck notifications@github.com schrieb am Do., 27. Sep. 2018, 07:56:

Hmm, I'm unsure how to hook a function in when google maps did indeed load. It happens internally https://github.com/tomchentw/react-google-maps/blob/master/src/withScriptjs.jsx#L60 so without changing the original source I don't see how it is possible.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/fable-compiler/fable-react/pull/102#issuecomment-424968010, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgNL6m6sfQGXSvKSGfel47YZFlE5dMks5ufGifgaJpZM4W7jWm .

nojaf commented 6 years ago

Guess the workaround is to use the LatLngLiteral instead of LatLng class. LatLngLiteral meaning { lat: 40, lng: 60 }, LatLng meaning new google.maps.LatLng(40,60).

forki commented 6 years ago

Yes we only need some way to deal with positions in the model. Also needs to work with server side rendering

Florian Verdonck notifications@github.com schrieb am Do., 27. Sep. 2018, 08:20:

Guess the workaround is to use the LatLngLiteral instead of LatLng class. LatLngLiteral meaning { lat: 40, lng: 60 }, LatLng meaning new google.maps.LatLng(40,60).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/fable-compiler/fable-react/pull/102#issuecomment-424972240, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgNPx9VjM1qD-xo2ykY6UfWgfj5OoZks5ufG44gaJpZM4W7jWm .

nojaf commented 6 years ago

Created https://github.com/fable-compiler/fable-import/pull/70

MangelMaxime commented 6 years ago

Hmm, I'm unsure how to hook a function in when google maps did indeed load.

Indeed it's unfortunate that we don't have a callback. We could probably mimic the code... But it's not ideal.

Another idea, it seems there a loadingElement, is it a react component? If yes, we could attach our callback in componentWillUnMount.

nojaf commented 6 years ago

Worth a shot, I'll investigate tonight.

nojaf commented 6 years ago

Once Fable.Import.GoogleMaps is a thing I believe this could be merged in. The only remaining mystery is why using LatLngBounds inside MapProperties.OnMapMounted onMapMounted is a problem.

It might be a webpack problems as using

[<Emit("new window.google.maps.LatLngBounds()")>]
let getBounds (): LatLngBounds = jsNative

    let onMapMounted ref =
        let mapRef = MapRef ref
        let bounds = getBounds ()

        markerPositions
        |> List.fold (fun (acc:LatLngBounds) pos -> acc.extend(!^ pos)) bounds
        |> mapRef.FitBounds

does work. When the code should run I'm certain that google.maps has been added to the DOM. It seems as webpack is already trying to evaluate the externals to replace. Or not, wild guesses.

I propose to merge if this works for @forki and I create an isolated sample with the problem.

nojaf commented 6 years ago

This is ready now, please review.

MangelMaxime commented 6 years ago

@forki Do you agree with the changes ? I am asking, because you are using this library :)

forki commented 6 years ago

Let's merge. I will try to update my stuff next week

Maxime Mangel notifications@github.com schrieb am Mi., 3. Okt. 2018, 11:24:

@forki https://github.com/forki Do you agree with the changes ? I am asking, because you are using this library :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fable-compiler/fable-react/pull/102#issuecomment-426569142, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgNIvZBkWxcyyrPXnc4eoHCIaona9cks5uhIJAgaJpZM4W7jWm .