ohanhi / elm-native-ui

[CLOSED] Experiment: mobile apps in Elm using React Native.
BSD 3-Clause "New" or "Revised" License
1.54k stars 76 forks source link

upgrade renderer to 0.18 and add full counter example #41

Closed lukewestby closed 7 years ago

lukewestby commented 7 years ago

Hi!

So I wrote a 0.18-compatible renderer that uses a similar tree to the VirtualDom renderer.

Some things that I'm still not sure about are:

Some things that are missing:

ohanhi commented 7 years ago

Thank you, Luke! This is the kind of love all experimental open source projects crave for. 💖

I think I'm gonna take a leap of faith here, and just do the merge as soon as I've tried the branch on my computer. Then I will move the repo back under my account on Github (since the Elm core and compiler forks are no longer needed) and start working on Fetch to get HTTP requests working.

Thanks again for restoring my faith in the continuation of this effort! 💯

ohanhi commented 7 years ago

Hmm, curious. The example only runs if I have the remote debugging enabled. Otherwise it will just show a blank white app. I suspect there is something to do in order to get a first render out of the app, and somehow enabling remote debugging does the trick.

lukewestby commented 7 years ago

Thank you, Luke! This is the kind of love all experimental open source projects crave for. 💖

I think I'm gonna take a leap of faith here, and just do the merge as soon as I've tried the branch on my computer. Then I will move the repo back under my account on Github (since the Elm core and compiler forks are no longer needed) and start working on Fetch to get HTTP requests working.

Thanks again for restoring my faith in the continuation of this effort! 💯

:heart: :heart: :heart: :heart:

Hmm, curious. The example only runs if I have the remote debugging enabled. Otherwise it will just show a blank white app. I suspect there is something to do in order to get a first render out of the app, and somehow enabling remote debugging does the trick.

I changed up the way Elm.Main.start() works so that it synchronously returns a component that you can pass to the AppRegistry. I don't really understand how AppRegistry works but it seems like React Native has some built-in race conditions around it. Seems to work now!

eeue56 commented 7 years ago

The commit size is way too big to review it via github's UI (it just crashes for me).

But here's some feedback on the elm native code:

Everything else looks the same as virtual-dom. There's a few changes that could be made, e.g using toArray instead of doing it by hand, but it's probably better to keep it as close as possible to core.

Either way, good stuff and only :art: changes!

lukewestby commented 7 years ago

https://github.com/lukewestby/elm-native-ui/blob/d7eb2eb4c885a216b784fdea378ea3ad14b54878/src/Native/NativeUi.js#L225 makes is really hard to guess what it's meant to be doing, so it could do with a comment

Done! I went through and commented every function.

https://github.com/lukewestby/elm-native-ui/blob/d7eb2eb4c885a216b784fdea378ea3ad14b54878/src/Native/NativeUi.js#L182 noValue doesn't really make sense in terms of a name for an object https://github.com/lukewestby/elm-native-ui/blob/d7eb2eb4c885a216b784fdea378ea3ad14b54878/src/Native/NativeUi.js#L218 this is better expressed by either using null or checking to see if there are no keys in the object

Usually I would reach for null here but a Json.Encode.Value where the value is null is technically a valid Elm model so I went for something guaranteed not to be coming from the Elm program. undefined should never show up in Elm programs so we can use that instead.

https://github.com/lukewestby/elm-native-ui/blob/d7eb2eb4c885a216b784fdea378ea3ad14b54878/src/Native/NativeUi.js#L190 you can avoid use of var self = this through using .bind(this) instead

self = this is an old habit 😄 I just remembered that React.createClass binds stuff automatically so I just moved everything into class methods.

the code in the treeToReactNative block is confusing - it would be clearer to move each switch statement out into their own function https://github.com/lukewestby/elm-native-ui/blob/d7eb2eb4c885a216b784fdea378ea3ad14b54878/src/Native/NativeUi.js#L145

Done! I also renamed the "node" node type to "component" internally so it would be more clear that it was pulling in a React component.

... using toArray instead of doing it by hand, but it's probably better to keep it as close as possible to core.

These functions originally did more stuff but now they're all just hand-rolled toArray so I updated them.

Thanks for the review, Noah!

lukewestby commented 7 years ago

@ohanhi @eeue56 do you think it'd be better not to check in all the react native stuff for the example app?

ohanhi commented 7 years ago

@lukewestby I'd prefer it to be a separate PR. That way this diff would be way smaller than it is now. 😄