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

Fable 1 support #20

Merged
merged 24 commits into from
Jun 30, 2017
Merged

Fable 1 support #20

merged 24 commits into from
Jun 30, 2017

Conversation

forki
Copy link
Member

@forki forki commented Jun 21, 2017

subsumes #19

@alfonsogarciacaro
Copy link
Collaborator

Ok, I close #19 👍 Things to do:

  • Check if everything works removing the Pojo attribute
  • Update Fable and libraries to 1.1.x (with Fable.Import.Browser split)
  • Fix build script

@forki
Copy link
Member Author

forki commented Jun 21, 2017

I'm working on the build

@forki
Copy link
Member Author

forki commented Jun 21, 2017

image

@alfonsogarciacaro what now?

@forki
Copy link
Member Author

forki commented Jun 21, 2017

@alfonsogarciacaro how is it supposed to work now?

@alfonsogarciacaro
Copy link
Collaborator

@forki I was just running Fable/Rollup and React Native in two different terminals:

#Terminal 1
dotnet fable npm-run build

#Terminal 2
react-native run-android

Remember to restore dependencies first :)

@forki
Copy link
Member Author

forki commented Jun 21, 2017

run fableTool "--verbose --symbols PRODUCTION" ""

how would this look like?

@alfonsogarciacaro
Copy link
Collaborator

In Fable 1.0 it's the fable-loader which sends the symbols/defineconstants to the Fable daemon. The tricky part is Webpack won't accept custom arguments (I think). Right now what we usually do is to detect if the -p argument has been passed to Webpack, and then pass the appropriate define symbols for each case. But I'm not sure with Rollup, I'll check.

The --verbose argument can be passed normally: dotnet fable npm-run build --verbose. See: dotnet fable --help for details.

@forki
Copy link
Member Author

forki commented Jun 21, 2017

good but I need to pass compiler directives as well

@alfonsogarciacaro
Copy link
Collaborator

Yes, you need to do it from the webpack.config.js (rollup.config.js in this case). It's not possible to pass compiler directives directly to Fable daemon because in theory the same daemon can be used to compile multiple projects with different configurations.

Anyways, don't worry, I'll do it later 👍

@forki
Copy link
Member Author

forki commented Jun 21, 2017

seems calling yarn install hangs for me

@forki
Copy link
Member Author

forki commented Jun 21, 2017

yarn install v0.23.2

@alfonsogarciacaro
Copy link
Collaborator

Bad karma ;) Maybe try updating yarn or use npm 5 (npm install -g npm to update)

@alfonsogarciacaro
Copy link
Collaborator

I've made some changes in #19, maybe you can rebase/merge them to this PR?

  • Update to Fable 1.1.3
  • Remove Pojo attribute
  • Detect if it's a production build in Rollup config (when you don't pass the -w argument) and in that case pass the PRODUCTION compiler directive to Fable). I've also changed the name of package.json script to start (watch/development) and build (production) to match other Fable templates 👍

@alfonsogarciacaro
Copy link
Collaborator

Apparently the latest commits are not showing in #19 PR as it's closed, you need to check branch fable-1-1 directly.

@forki
Copy link
Member Author

forki commented Jun 22, 2017

image

@forki
Copy link
Member Author

forki commented Jun 22, 2017

wtf? stying like this forever

@forki
Copy link
Member Author

forki commented Jun 27, 2017

running chcp 850 resolved it

@forki
Copy link
Member Author

forki commented Jun 27, 2017

@alfonsogarciacaro now I'm lost:

image

@inosik
Copy link
Contributor

inosik commented Jun 27, 2017

@forki I think I ran into this once, when I tried to fix fsprojects/FAKE#1461, at least I had the same stack trace.

Node.js seems to have a problem with the Console.OutputEncoding change we do in FAKE to prevent the Chinese characters bug on Mono.

Does the build script output any Unicode characters? I triggered this crash when printing the copyright character to the console.

@forki
Copy link
Member Author

forki commented Jun 29, 2017

./node_modules/.bin/react-native run-android doesn't work
.\node_modules\.bin\react-native run-android doesn't work

react-native.cmd run-android does work a bit further:

 A problem occurred configuring project ':react-native-onesignal'.

which is probably a real error

@alfonsogarciacaro
Copy link
Collaborator

For the record, in my tests I was using the react-native command installed globally (by running npm i -g react-native-cli) as this is what the React Native tutorial recommends.

@forki
Copy link
Member Author

forki commented Jun 29, 2017

crazy! I got it working. Only one issue: Fable doesn't go to watch mode. Ideas?

@inosik
Copy link
Contributor

inosik commented Jun 29, 2017

Call dotnet fable npm-run start instead of ... build from FAKE?

@forki
Copy link
Member Author

forki commented Jun 29, 2017

ok that works. Fable is in watch mode and runs on device.

two remaining issue:

a) ionide is unhappy and show lots of errors /cc @Krzysztof-Cieslak
b) for normal build we need to set production flag for compiler directive

@forki forki requested a review from Krzysztof-Cieslak June 29, 2017 12:48
@inosik
Copy link
Contributor

inosik commented Jun 29, 2017

You can revert 94fffb5.

@enricosada
Copy link

enricosada commented Jun 29, 2017

@forki atm ionide and fsac and sdk are solid :D

dotnt build

       "e:\github\fable-react_native-demo\Nightwatch.fsproj" (Build target) (1) ->
       (CoreCompile target) ->
         C:\dotnetcli\dotnet-dev-win-x64.1.0.4\sdk\1.0.4\Sdks\FSharp.NET.Sdk\Sdk\Sdk.targets(27,5): warning : The 'CoreCompile' target should be overriden in 'FSharp.NET.Sdk' package. Maybe you need to add 'FSharp.NET.Sdk' package to fsproj or run restore [e:\github\fable-react_native-demo\Nightwatch.fsproj]

after adding <PackageReference Include="FSharp.NET.Sdk" Version="1.0.*" /> works

restorerequired4

gif is wip of new restore if not already + progress indicators for that (at end more restore asked, because fable doesnt do project references to these otherwise np, i'll take that in consideration too for ionide)

@forki
Copy link
Member Author

forki commented Jun 29, 2017

ok ionide fixed with help of @enricosada and @Krzysztof-Cieslak

@forki
Copy link
Member Author

forki commented Jun 29, 2017

so last thing on the list: compiler directives

@forki
Copy link
Member Author

forki commented Jun 29, 2017

ok compiler directives work as well. yay!

one last thing:

let result =
    ExecProcess (fun info ->
        info.FileName <- dotnetExePath
        info.WorkingDirectory <- srcDir
        info.Arguments <- " fable npm-run build") TimeSpan.MaxValue
if result <> 0 then failwith "fable shut down."

it seems that a compiler error doesn't make this quit with exit code <> 0 - therefor build continues.

@alfonsogarciacaro ideas?

@alfonsogarciacaro
Copy link
Collaborator

This is tricky. Before Fable 1.0, compilation would stop after encountering the first error. I changed this so you could get multiple errors and show them in the IDE (see here). For bundler it's also important not to stop compilation, because then file dependencies won't be listed and thus won't be watched for changes either.

I think this is working fine with Webpack, but Rollup had a problem because errors would stop compilation so I had to report everything as a warning.

Probably we can work around this by adding a failOnError option or similar to the Rollup plugin to throw actual errors if it's not a watch compilation, what do you think?

@forki
Copy link
Member Author

forki commented Jun 30, 2017

yes I only need to let it fail on "normal" build withoutt watch, But we need to make it so otherwise CI would just go on and create corrupt apk

@alfonsogarciacaro
Copy link
Collaborator

I've publish rollup-plugin-fable 1.0.6, can you please update and change this line as follows?

  plugins: [fable({extra: { failOnError: true })],

@forki
Copy link
Member Author

forki commented Jun 30, 2017

seems to work! thx

@forki forki merged commit df9f8ae into master Jun 30, 2017
@forki forki deleted the fable1 branch June 30, 2017 16:05
@forki
Copy link
Member Author

forki commented Jun 30, 2017

g0w4e

wow Fable 1 is now working on my device as expected!!!!

Thanks so much everyone

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.

4 participants