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

Adding Babel types #1615

Merged
merged 8 commits into from
May 6, 2020
Merged

Adding Babel types #1615

merged 8 commits into from
May 6, 2020

Conversation

ncave
Copy link
Collaborator

@ncave ncave commented Oct 22, 2018

This is trying to bring back some of the Babel types from Fable 1.x

@alfonsogarciacaro A few questions, if you have the time to take a quick look:

  • Do we need to keep generic arg list in AST.Fable.FunctionKind and AST.Fable.ObjectMemberKind?
  • Obviously more types need to be properly extracted and set, e.g. for arrow func expressions, etc.
  • Anything else that catches your eye.

@alfonsogarciacaro
Copy link
Member

This is great @ncave! I'll try to have a deeper look later. But sorry, I don't understand your question, AST.Fable.FunctionKind keeps a list of argument Idents and AST.Fable.ObjectMemberKind doesn't keep any reference to the arguments at all (they must be contained in a Function expression within member value). What do you need exactly?

@ncave
Copy link
Collaborator Author

ncave commented Oct 22, 2018

@alfonsogarciacaro Previously (in 1.x) the getMemberArgsAndBody was getting the generic types as an additional parameter, and it needs to come from some func or member info in AST.Fable (or not, if you don't think so). It's used here.

@alfonsogarciacaro
Copy link
Member

Ah, ok! I think probably the generic arguments can be extracted from the function type but let me check.

@alfonsogarciacaro
Copy link
Member

@ncave Sorry, forgot about this one! 😅 I haven't tested it, but I guess if you have the arg idents and the body you can get the generic parameter names for the annotation like this:

        let genParams =
            let rec getGenParams = function
                | Fable.GenericParam name -> [name]
                | t -> t.Generics |> List.collect getGenParams
            let types = (args |> List.map (fun (i: Fable.Ident) -> i.Type)) @ [body.Type]
            ([], types) ||> List.fold (fun acc t ->
                acc @ (getGenParams t))

Does it help?

@ncave
Copy link
Collaborator Author

ncave commented Nov 19, 2018

@alfonsogarciacaro Any idea what can we do here for entities, as typeRef doesn't exist anymore?

@alfonsogarciacaro
Copy link
Member

@ncave Probably you can use entityRefMaybeImported which is used to get the constructor function reference. But it doesn't work for the "primitive" types. We'll probably need a pattern matching like this one to cover all cases.

@MangelMaxime
Copy link
Member

you could be targeting other compilers (typescript, babel, flow, assemblyscript etc.).

Do you mean with the current implementation we can target the other compilers? I am asking because it is the reason why I wanted to make it explicit that we are generating typescript as an output.

I didn't want to have --typed for now and then we a new target is added have --typed for typescript and --flow for flow.

I hope my intention is clear ^^

@ncave
Copy link
Collaborator Author

ncave commented Mar 12, 2020

@MangelMaxime Yes, we should be able to target other compilers, but the name of the compiler option is not that important to spend much time over it. We can change it to be --typescript.

@ncave ncave force-pushed the flow branch 2 times, most recently from 92d4935 to 957fc4b Compare March 18, 2020 21:48
@ncave ncave force-pushed the flow branch 3 times, most recently from 6610697 to 0d0b6ca Compare April 14, 2020 22:23
@ncave
Copy link
Collaborator Author

ncave commented Apr 14, 2020

Rebased to latest master.

@ncave ncave force-pushed the flow branch 3 times, most recently from f332cd6 to 303ca59 Compare April 18, 2020 23:51
@ncave ncave changed the title [WIP] Adding Babel types Adding Babel types Apr 19, 2020
@ncave
Copy link
Collaborator Author

ncave commented Apr 19, 2020

@MangelMaxime As mentioned above, this functionality is hidden behind a flag, so it's fine to merge it in Fable v2.x. But if you prefer, we can merge this into a Fable v3.0 branch and publish it with a 3.0 prerelease tag (alpha/beta/next).

@MangelMaxime
Copy link
Member

@ncave Sorry for the delay, I got a bit crushed by work lately.

I will include it in Fable 2 as you mentioned it is being a flag. I am also planning on making a pass on all Fable files to fix the indentation because some files, use 2 spaces,3 spaces, 4 spaces, or a mix of both.

And, if we put your work in a separate branch it will make it even harder to fix the indentation problems^^.

Also, with your current work on making us use fable-spliter to build test etc. We should be able to add a new test target for typescript generation.

@MangelMaxime
Copy link
Member

@ncave I am not forgetting your PR.

I am working on it while doing so I am also learning how Fable, fable-loader, fable-splitter and co works. So it will probably a bit more time for me to merge this PR.

Sorry for the delay, I am exploring areas I never explored before in Fable and they are not the easiest to understand right now ^^

@ncave
Copy link
Collaborator Author

ncave commented Apr 24, 2020

@MangelMaxime No worries, let me know if I can help with something (if I know the answer).

@MangelMaxime
Copy link
Member

@ncave Thank you.

I think I am starting to understand how things work. I will probably create a diagram to help contribute and document the infra when re-working Fable website.

Right now, I almost have a version of fable-splitter working with your branch:

image

Almost, because I have a bug when copying the fable-library-ts source files. When fixed I will share my work so we can review it.

I also saw a bug in the type generation when generating partial application:

printfn "%s : %s" "maxime" "mangel"

generates:

(function () {
  const clo1: (arg0: string, arg1: string) => void = toConsole(printf("%s : %s"));
  const clo2: (arg0: string) => void = clo1("maxime");
  clo2("mangel");
})();

image

I don't know if that's the correct definition but if I manually change the type to:

const clo1: (arg0: string) =>  (arg1: string) => void = toConsole(printf("%s : %s"));

then the error disappears.

@ncave
Copy link
Collaborator Author

ncave commented Apr 26, 2020

@MangelMaxime Yes, partial application is one of the few things that don't work properly yet. Producing types is still an experimental feature and needs to stay behind a flag.

Another one is union/record/value type construction, still debatable if we should stick to object constructor functions, or generate minimal classes and use new (only when types are enabled, otherwise it's pretty clear from benchmarking that object constructor functions have performance benefit over creating class instances with new).

Still, it should be fine to merge it as is and improve as we go, but I'll leave it up to you.

@ncave
Copy link
Collaborator Author

ncave commented May 5, 2020

Also, this PR makes some steps towards resolving #1887.

@MangelMaxime
Copy link
Member

@ncave Ahah you were quicker than me to rebase your branch

And here we go it is time to press the "red button"

@MangelMaxime MangelMaxime merged commit 99bc5a4 into fable-compiler:master May 6, 2020
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.

None yet

3 participants