WIP: Use a proper main #24

Merged
merged 1 commit into from Mar 16, 2016

Conversation

3 participants
@eeue56
Contributor

eeue56 commented Mar 11, 2016

Based off of @ohanhi's discussion with Evan, we can use a fork of elm-compiler with this project. This fork can be found here. Binaries will be coming soon for those who ask.

The premise is that all bootstrapping code should be moved into a Elm Native module.

It's likely that Core will also needed to be forked. I can do that too, if someone breaks down for me:

  • What the init stage is
  • What the update loop is
  • How things get drawn
@ohanhi

This comment has been minimized.

Show comment
Hide comment
@ohanhi

ohanhi Mar 12, 2016

Owner

@eeue56 My goal is to make the whole thing analogous with how elm-html works. In my mind, StartApp should work unmodified with this. Things currently get drawn in the index.ios.js, basically here. Does this answer your questions?

Owner

ohanhi commented Mar 12, 2016

@eeue56 My goal is to make the whole thing analogous with how elm-html works. In my mind, StartApp should work unmodified with this. Things currently get drawn in the index.ios.js, basically here. Does this answer your questions?

@yusefnapora

This comment has been minimized.

Show comment
Hide comment
@yusefnapora

yusefnapora Mar 12, 2016

Contributor

I think that a slight modification to StartApp will be needed, since we'd want to return a Signal VTree instead of a Signal Html as StartApp currently does. But I think that's probably all that needs to change.

Contributor

yusefnapora commented Mar 12, 2016

I think that a slight modification to StartApp will be needed, since we'd want to return a Signal VTree instead of a Signal Html as StartApp currently does. But I think that's probably all that needs to change.

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Mar 12, 2016

Contributor

making startapp work is trivial, just need to use the NRI fork. The bit that is complicated the bootstrapping bit in the runtime file

Contributor

eeue56 commented Mar 12, 2016

making startapp work is trivial, just need to use the NRI fork. The bit that is complicated the bootstrapping bit in the runtime file

@yusefnapora

This comment has been minimized.

Show comment
Hide comment
@yusefnapora

yusefnapora Mar 13, 2016

Contributor

@eeue56 I managed to get a working implementation last night. It's at my vtree-main branch and depends on this fork of elm-core.

It uses a very similar technique as the Elm virtual-dom package, in that the Node type is backed directly by React elements, rather than being an ADT which gets converted into React elements later.

I think it might be possible to get the ADT type to work as well, but it would require a lot of awkward javascript to inspect the vtree's ctor and extract the values, etc. since main is returning the VTree type directly without encoding to json. I might be overlooking a simpler approach though :)

The elm-core fork adds an Elm.embedReact() function that works similarly to Elm.embed(). It has three required args:

  • the elm module to make and run, e.g. Elm.Main
  • the instantiated React container component to push the vtree into
  • an object containing a React field, whose value is the output of require('react-native')
    • you can also pass in your own custom (or third-party) javascript components to expose them to elm

And you can pass in a fourth argument, which is the optional object of inputs as with Elm.fullscreen and Elm.embed

Probably needs some cleanup before it's ready to go, but hopefully this'll be a decent start :)

Contributor

yusefnapora commented Mar 13, 2016

@eeue56 I managed to get a working implementation last night. It's at my vtree-main branch and depends on this fork of elm-core.

It uses a very similar technique as the Elm virtual-dom package, in that the Node type is backed directly by React elements, rather than being an ADT which gets converted into React elements later.

I think it might be possible to get the ADT type to work as well, but it would require a lot of awkward javascript to inspect the vtree's ctor and extract the values, etc. since main is returning the VTree type directly without encoding to json. I might be overlooking a simpler approach though :)

The elm-core fork adds an Elm.embedReact() function that works similarly to Elm.embed(). It has three required args:

  • the elm module to make and run, e.g. Elm.Main
  • the instantiated React container component to push the vtree into
  • an object containing a React field, whose value is the output of require('react-native')
    • you can also pass in your own custom (or third-party) javascript components to expose them to elm

And you can pass in a fourth argument, which is the optional object of inputs as with Elm.fullscreen and Elm.embed

Probably needs some cleanup before it's ready to go, but hopefully this'll be a decent start :)

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Mar 13, 2016

Contributor

👍 looks good

Contributor

eeue56 commented Mar 13, 2016

👍 looks good

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Mar 13, 2016

Contributor

an object containing a React field, whose value is the output of require('react-native')

I don't think this is a good idea. If you want to require things like this, just use import Native.ReactNativeJS instead, where ReactNativeJS is the JS file to be imported

Contributor

eeue56 commented Mar 13, 2016

an object containing a React field, whose value is the output of require('react-native')

I don't think this is a good idea. If you want to require things like this, just use import Native.ReactNativeJS instead, where ReactNativeJS is the JS file to be imported

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Mar 13, 2016

Contributor

It uses a very similar technique as the Elm virtual-dom package, in that the Node type is backed directly by React elements, rather than being an ADT which gets converted into React elements later.

One of the biggest problems with this approach is that you can no longer use things like toString or show on the vdom elements, leading to runtime errors. Since there currently is no elm-native-ui legacy projects, you should instead try a different approach IMO. This is a lesson I learnt from doing server-side Elm, where I needed to debug things a lot. Having toString work or other things we expect from Elm is very handy :)

Contributor

eeue56 commented Mar 13, 2016

It uses a very similar technique as the Elm virtual-dom package, in that the Node type is backed directly by React elements, rather than being an ADT which gets converted into React elements later.

One of the biggest problems with this approach is that you can no longer use things like toString or show on the vdom elements, leading to runtime errors. Since there currently is no elm-native-ui legacy projects, you should instead try a different approach IMO. This is a lesson I learnt from doing server-side Elm, where I needed to debug things a lot. Having toString work or other things we expect from Elm is very handy :)

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Mar 13, 2016

Contributor

Also, grab https://github.com/NoRedInk/elm-compiler/tree/elm-native-ui and see if it works and let me know if it doesn't

Contributor

eeue56 commented Mar 13, 2016

Also, grab https://github.com/NoRedInk/elm-compiler/tree/elm-native-ui and see if it works and let me know if it doesn't

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Mar 13, 2016

Contributor

Also - the diff is very noisy! Can you make it without the elm-format changes?

Contributor

eeue56 commented Mar 13, 2016

Also - the diff is very noisy! Can you make it without the elm-format changes?

@yusefnapora

This comment has been minimized.

Show comment
Hide comment
@yusefnapora

yusefnapora Mar 13, 2016

Contributor

Sweet, thanks for checking it out :)

I think you're right that backing with react elements directly is buying trouble down the road... I went that route because I was using virtual-dom as a reference, but I'd rather keep things in "elm land" as much as possible.

I'll take another pass through and try to get the ADT approach working, and hopefully get rid of the need to pass in the reactEnvironment object with the require'd React components.

Sorry about the noise, I'll switch off elm-format and try splitting the diffs a bit for the next round :)

Contributor

yusefnapora commented Mar 13, 2016

Sweet, thanks for checking it out :)

I think you're right that backing with react elements directly is buying trouble down the road... I went that route because I was using virtual-dom as a reference, but I'd rather keep things in "elm land" as much as possible.

I'll take another pass through and try to get the ADT approach working, and hopefully get rid of the need to pass in the reactEnvironment object with the require'd React components.

Sorry about the noise, I'll switch off elm-format and try splitting the diffs a bit for the next round :)

@yusefnapora

This comment has been minimized.

Show comment
Hide comment
@yusefnapora

yusefnapora Mar 13, 2016

Contributor

Btw, the forked compiler worked great; I just had to update the elm-package.json's repository field to https://github.com/elm-native-ui/elm-native-ui.git so the compiler would be happy.

Contributor

yusefnapora commented Mar 13, 2016

Btw, the forked compiler worked great; I just had to update the elm-package.json's repository field to https://github.com/elm-native-ui/elm-native-ui.git so the compiler would be happy.

@yusefnapora

This comment has been minimized.

Show comment
Hide comment
@yusefnapora

yusefnapora Mar 13, 2016

Contributor

I made a new branch that keeps the VTree type as an elm ADT until it hits the Native.ReactNative.render function, which is only accessed by elm-core's Runtime.js. So inside of Elm, you should be able to call toString, etc. on a VTree.

I also removed the reactEnvironment arg to Elm.embedReact in the elm-core fork. This means that there's not yet a way to render custom JS components, but I'm fine with leaving that out until a clean solution is found.

There's still one opaque, native-backed type: NativeValue is used for event handler functions, and there's a NativeProperty data constructor that accepts a NativeValue. I can't think of another way to attach event handlers directly to the VTree type... But if there's a cleaner way, let's go for it.

Contributor

yusefnapora commented Mar 13, 2016

I made a new branch that keeps the VTree type as an elm ADT until it hits the Native.ReactNative.render function, which is only accessed by elm-core's Runtime.js. So inside of Elm, you should be able to call toString, etc. on a VTree.

I also removed the reactEnvironment arg to Elm.embedReact in the elm-core fork. This means that there's not yet a way to render custom JS components, but I'm fine with leaving that out until a clean solution is found.

There's still one opaque, native-backed type: NativeValue is used for event handler functions, and there's a NativeProperty data constructor that accepts a NativeValue. I can't think of another way to attach event handlers directly to the VTree type... But if there's a cleaner way, let's go for it.

@ohanhi

This comment has been minimized.

Show comment
Hide comment
@ohanhi

ohanhi Mar 14, 2016

Owner

The newest iteration is looking awesome, @yusefnapora! Great work! Could we maybe get this stuff merged within a couple of days, so the live coding session could already be based on the new solution? :)

Owner

ohanhi commented Mar 14, 2016

The newest iteration is looking awesome, @yusefnapora! Great work! Could we maybe get this stuff merged within a couple of days, so the live coding session could already be based on the new solution? :)

@yusefnapora

This comment has been minimized.

Show comment
Hide comment
@yusefnapora

yusefnapora Mar 14, 2016

Contributor

That sounds good to me @ohanhi :) I'll hopefully have some time tonight to go over the comments you and @eeue56 made and do a bit of cleanup.

Contributor

yusefnapora commented Mar 14, 2016

That sounds good to me @ohanhi :) I'll hopefully have some time tonight to go over the comments you and @eeue56 made and do a bit of cleanup.

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Mar 16, 2016

Contributor

@yusefnapora is there anything I need to do to help or have you got it?

Contributor

eeue56 commented Mar 16, 2016

@yusefnapora is there anything I need to do to help or have you got it?

@yusefnapora

This comment has been minimized.

Show comment
Hide comment
@yusefnapora

yusefnapora Mar 16, 2016

Contributor

@eeue56 if you want to review the last few commits, that'd be sweet. I put up #25 to collect the diffs and make it a bit easier to track changes. I think it's in pretty decent shape, but it would be nice to update the README with a note about the compiler and elm-core forks.

Thanks for your earlier comments & reviews, btw; they were very helpful!

Contributor

yusefnapora commented Mar 16, 2016

@eeue56 if you want to review the last few commits, that'd be sweet. I put up #25 to collect the diffs and make it a bit easier to track changes. I think it's in pretty decent shape, but it would be nice to update the README with a note about the compiler and elm-core forks.

Thanks for your earlier comments & reviews, btw; they were very helpful!

ohanhi added a commit that referenced this pull request Mar 16, 2016

@ohanhi ohanhi merged commit 6042931 into ohanhi:master Mar 16, 2016

@eeue56 eeue56 deleted the eeue56:patch-2 branch Mar 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment