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

[WIP] Update to Fable 2 #66

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

[WIP] Update to Fable 2 #66

wants to merge 14 commits into from

Conversation

forki
Copy link
Member

@forki forki commented Feb 20, 2019

@forki forki mentioned this pull request Feb 20, 2019
@@ -17,7 +17,7 @@ module.exports = {
entry: resolve('src/Nightwatch.fsproj'),
outDir: resolve("out"),
babel: {
// presets: [["es2015", { modules: false }]],
plugins: ["@babel/plugin-transform-modules-commonjs"],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the example, I have you I use @babel/plugin-transform-modules-commonjs because it was needed for node.js support

I don't know, if this is needed for React Native env.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok. let me remove it for now

@MangelMaxime
Copy link

@forki What are you expecting from me?

For me, the diff seems good for an upgrade to Fable 2.1. Do you expect me to build/test the app?

@forki
Copy link
Member Author

forki commented Feb 20, 2019

no I don't.
I'm currently missing the command that will run fable. #64 (comment)
basically the replacement for dotnet fable npm-run build

@MangelMaxime
Copy link

You need to start fable-splitter by using yarn run fable-splitter for example.

It will start the compiler for you.

@forki
Copy link
Member Author

forki commented Feb 20, 2019

ah cool. that was missing link. trying that now!

@forki
Copy link
Member Author

forki commented Feb 21, 2019

So everything compiles, but

screenshot_20190221-121514

react-navigation/react-navigation#2767 (comment) suggest there is some trouble with the import.

https://stackoverflow.com/a/47871724/145701 suggests there needs to be some additonal export!?

But I don't see what changed.

@alfonsogarciacaro any ideas?

@forki
Copy link
Member Author

forki commented Feb 21, 2019

@MangelMaxime I assume we need to "export" Elmish.ReactNative.Components.App somehow!?

https://github.com/elmish/react/blob/master/src/react-native.fs#L15

@forki
Copy link
Member Author

forki commented Feb 21, 2019

elmish/react@c6c15b5#diff-ce3e72f76b021b9de55e8aba30e99cf5R45 looks like @alfonsogarciacaro already fixed something for fable2. how can I test that?

@forki
Copy link
Member Author

forki commented Feb 21, 2019

yes that's the correct change. good work @alfonsogarciacaro
just the issue is: there is no compatible package of this released

@MangelMaxime
Copy link

@forki If I didn't mess up the merge/rebase, I released Fable.Elmish.React 2.2.0 with the fix included.

Available when nuget ready.

@MangelMaxime
Copy link

@MangelMaxime I assume we need to "export" Elmish.ReactNative.Components.App somehow!?

https://github.com/elmish/react/blob/master/src/react-native.fs#L15

I don't understand this comment. Is the fix in Fable.Elmish.React enough or do we still need to do something?

@forki
Copy link
Member Author

forki commented Feb 21, 2019

fix would be enough. let me test the new packge

@forki
Copy link
Member Author

forki commented Feb 21, 2019

confirmed. Fable.Elmish.React 2.2.0 fixed it. thanks!

now I'm down to warnings.

screenshot_20190221-133821

it complains about line 59 in

image

@forki
Copy link
Member Author

forki commented Feb 21, 2019

 const logo = createElement(Image, createObj(ofArray([new Props.ImageProperties(5, require("../../images/raven.jpg")), new Props.ImageProperties(6, ofArray([new Props.FlexStyle(1, "center"), new Props.FlexStyle(10, "column")]))]), 1));

this is how fable did it in 1.x

@alfonsogarciacaro
Copy link
Collaborator

Looks like the keyValueProps call is missing. Fable 1 did this implicitly I believe, but in Fable 2 you have to call it explicitly to transform the list into a JS object

@alfonsogarciacaro
Copy link
Collaborator

Ah, I think the Style function is wrong, I'll check after coming back from kindergarten

@forki
Copy link
Member Author

forki commented Feb 22, 2019

so we need to update the bindings?

@alfonsogarciacaro
Copy link
Collaborator

I think so, in fable-react, Style is disguised as a union case, but it's actually a function that calls keyValueList: https://github.com/fable-compiler/fable-react/blob/b89835014ae16caec51938ad8337010b36b13613/src/Fable.React/Fable.React.Props.fs#L731-L732

We need to do something similar for fable-react-native bindings, for example here: https://github.com/fable-compiler/fable-react-native/blob/e4c666f27dfc1ff77bdbfaa0e899f30dadff2839/src/Fable.Helpers.ReactNative.fs#L531

Because there are many cases accepting IStyle list, if this makes the bindings too hacky, another solution is to create a function like this:

type IStyleProps = interface end

let style (props: IStyle seq) = keyValueList CaseRules.LowerFirst props :?> IStyleProps

Then change all IStyle list to IStyleProps. The drawback is it forces the user to call style all the time, but you can turn the function into an operator like: let (!^) (props: IStyle seq) = ...

@forki
Copy link
Member Author

forki commented Feb 22, 2019 via email

@forki
Copy link
Member Author

forki commented Feb 22, 2019

let's go with option 1: what do I need to do?

@forki
Copy link
Member Author

forki commented Feb 22, 2019

@alfonsogarciacaro

for testing I added the following to my Fable.Helpers.ReactNative.fs:

let inline ImagePropertiesStyle (style: IStyle list) : IImageProperties =
    !!("style", keyValueList CaseRules.LowerFirst style)

and indeed it seems to work if I use that function instead of ImageProperties.Style.

so we replace all these union cases with such a function? Is that the idea?

@MangelMaxime
Copy link

@forki Yes, the idea is to replace the *.Style to use a similar function.

If the existing DSL used a Style case, you can use a static member and make it a non breaking change. Example

@forki
Copy link
Member Author

forki commented Feb 22, 2019 via email

@forki
Copy link
Member Author

forki commented Feb 22, 2019

@alfonsogarciacaro @MangelMaxime looks like we actually made it work. Thanks guys!

Only issue left is bug in fable splitter: fable-compiler/Fable#1751

forki and others added 4 commits February 26, 2019 14:07
Major changes:

- Upgrade to RN 58.5
- Upgrade the gradle/android project to latest version (seems needed for RN 58.5)
- Upgrade to Elmish.HMR 3.2 which fix HMR support for RN
- Re-activate the HMR support in the application
@ImaginaryDevelopment
Copy link

Is this work still ongoing? Can we use DotNet 5 and the latest Fable for react-native apps?

@tforkmann
Copy link
Collaborator

I have a open PR for net5 and Fable 3
Works on iOS
Android need a tiny fix

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.

5 participants