Return VTree from main #25

Merged
merged 12 commits into from Mar 16, 2016

Conversation

3 participants
@yusefnapora
Contributor

yusefnapora commented Mar 15, 2016

Hey, I figured it would make discussion / review easier if I make a separate PR, rather than continuing to comment on #24

To reacap: this depends on this fork of the elm compiler, which must be on your PATH before the standard elm compiler.

You also need to replace the elm-stuff/packages/elm-lang/core/3.0.0 directory with this fork of elm-core, which adds an Elm.embedReact function to enable rendering for the VTree type. Make sure you checkout the elm-native-ui branch when you clone it, btw.

I went ahead and converted the Native code to ES5 syntax, since it doesn't need any fancy ES6 goodies, and it's nice to have a consistent style. However, it does still use Object.assign, so it's not completely free of ES6 runtime dependencies.

I also tried to generally clean things up a bit and added some comments to ReactNative.elm

Let me know if there's anything else that needs doing :)

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Mar 13, 2016

returning undefined is suspicious

returning undefined is suspicious

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Mar 13, 2016

Generally against using things which won't work in a browser. Vanilla js probably is best JS for Elm

Generally against using things which won't work in a browser. Vanilla js probably is best JS for Elm

This comment has been minimized.

Show comment
Hide comment
@ohanhi

ohanhi Mar 14, 2016

Generally agreed. But this is very React Native -specific implementation, so I think ES6 is fine.

Generally agreed. But this is very React Native -specific implementation, so I think ES6 is fine.

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Mar 13, 2016

👍 you should also probably include a shim for require

👍 you should also probably include a shim for require

This comment has been minimized.

Show comment
Hide comment
@yusefnapora

yusefnapora Mar 13, 2016

Owner

I'm actually not sure it's possible, although if it is, that'd be great. The react native packager's require implementation is kind of heavy-handed... As far as I can tell it just does a string match against the token require, and you have to pass a raw string into that function for the dependency to be picked up by the packager.

So, e.g. this won't work, since the packager can't know the value of pkg statically:

var pkg = './foo/bar';
require(pkg);

Renaming or otherwise wrapping require also breaks the dependency resolver. So does this, actually:

var foo = "require('./does/not/exist')";

which is why I think it's basically a regex, since it doesn't notice that the require is inside a string literal...

Owner

yusefnapora replied Mar 13, 2016

I'm actually not sure it's possible, although if it is, that'd be great. The react native packager's require implementation is kind of heavy-handed... As far as I can tell it just does a string match against the token require, and you have to pass a raw string into that function for the dependency to be picked up by the packager.

So, e.g. this won't work, since the packager can't know the value of pkg statically:

var pkg = './foo/bar';
require(pkg);

Renaming or otherwise wrapping require also breaks the dependency resolver. So does this, actually:

var foo = "require('./does/not/exist')";

which is why I think it's basically a regex, since it doesn't notice that the require is inside a string literal...

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Mar 13, 2016

Ah, I see, it's something that ReactNative does differently? Here I use a shim - https://github.com/NoRedInk/take-home/blob/master/src/Native/Telate.js#L41

then I can include JS files like this:

module Moment where

import Native.MomentJS
import Native.Moment

then I can use require('moment') in my Native implementation of the Moment stuff.

Ah, I see, it's something that ReactNative does differently? Here I use a shim - https://github.com/NoRedInk/take-home/blob/master/src/Native/Telate.js#L41

then I can include JS files like this:

module Moment where

import Native.MomentJS
import Native.Moment

then I can use require('moment') in my Native implementation of the Moment stuff.

This comment has been minimized.

Show comment
Hide comment
@yusefnapora

yusefnapora Mar 13, 2016

Owner

Yeah, the RN require isn't just a function, unfortunately. I'm pretty sure the rationale is that the packager builds a dependency graph so that it knows what to bundle, and it does so before the code is evaluated. I assume that the since the packager is running in node, its execution environment is too different from the actual RN runtime environment for a dynamic require implementation to work.

It's kind of a drag, but I don't think it's a big priority for the RN team.

Owner

yusefnapora replied Mar 13, 2016

Yeah, the RN require isn't just a function, unfortunately. I'm pretty sure the rationale is that the packager builds a dependency graph so that it knows what to bundle, and it does so before the code is evaluated. I assume that the since the packager is running in node, its execution environment is too different from the actual RN runtime environment for a dynamic require implementation to work.

It's kind of a drag, but I don't think it's a big priority for the RN team.

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Mar 13, 2016

Yeah, that's fair. I guess there are some things that are different about RN that I don't know about

Yeah, that's fair. I guess there are some things that are different about RN that I don't know about

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Mar 13, 2016

semicolon

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Mar 13, 2016

Not sure about this name, since it doesn't convert a list to JS - it converts it to an object

Not sure about this name, since it doesn't convert a list to JS - it converts it to an object

@yusefnapora yusefnapora referenced this pull request Mar 16, 2016

Merged

WIP: Use a proper main #24

+The `tagName` will be used to look up a React Component class with the same name,
+so e.g. `node "View"` will render a React Native `View` component.
+-}
+node : String -> List Property -> List VTree -> VTree

This comment has been minimized.

@ohanhi

ohanhi Mar 16, 2016

Owner

I really like this. This kind of a setup was always the end goal for us. :)

@ohanhi

ohanhi Mar 16, 2016

Owner

I really like this. This kind of a setup was always the end goal for us. :)

@@ -0,0 +1,41 @@
+'use strict';

This comment has been minimized.

@ohanhi

ohanhi Mar 16, 2016

Owner

I guess we can use the same wrapper for both platforms all the time, yeah. There was a reason why we didn't do this straight in the beginning, though: usually bigger React Native apps have differences between iOS and Android versions. There are components that only work on iOS and vice versa. So in the long run, we should figure out a way to support this, maybe simply by having a MainAndroid.elm and MainIos.elm, which would be compiled separately to e.g. elm.android.js and elm.ios.js.

@ohanhi

ohanhi Mar 16, 2016

Owner

I guess we can use the same wrapper for both platforms all the time, yeah. There was a reason why we didn't do this straight in the beginning, though: usually bigger React Native apps have differences between iOS and Android versions. There are components that only work on iOS and vice versa. So in the long run, we should figure out a way to support this, maybe simply by having a MainAndroid.elm and MainIos.elm, which would be compiled separately to e.g. elm.android.js and elm.ios.js.

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

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

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