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

Use library-ts also for JS compilation #3420

Merged
merged 6 commits into from
Apr 15, 2023
Merged

Conversation

alfonsogarciacaro
Copy link
Member

This unifies fable-library(-js) and fable-library-ts. The idea is to publish fable-library as JS but with .d.ts declarations to make sure F# code compiled both as JS and TS is compatible.

I ama having some trouble with the const enums for the union tags, so we may need to drop them and use plain ints again.

@ncave
Copy link
Collaborator

ncave commented Apr 12, 2023

@alfonsogarciacaro Yes, it will be great to have .d.ts in fable-library, but we still want to keep the fable-library-ts for lang=TypeScript, right? For cases where pure TS output is preferred?

@alfonsogarciacaro
Copy link
Member Author

Yes, when embedding the fable-library-ts files we can keep doing it the same way. The use case I've in mind is publishing fable-library, then you write a library in F# with the fable-library dependency and compile it to Typescript, then to .js + .d.ts for publishing. And finally you can consume it either from a JS or TS project.

@alfonsogarciacaro
Copy link
Member Author

As commented above, finally this PR is giving up using const enum to represent union tags. This is because it caused problems with isolated modules which apparently is a requirement of many alternate TS compilers used in different toolchains. On the plus side, now TS consumers can also safely pattern match against the union name.

I have added casts in several places to make things work. @ncave I have also included your code from #3411 in the tests to make sure it compiles with TypeScript, but it'd be great if you could add some actual assertions.

@ncave
Copy link
Collaborator

ncave commented Apr 14, 2023

@alfonsogarciacaro It's not my code actually, it's a heavily reduced repro sourced from FCS.
I'm not sure including it is a good test, unless we also test for file size or compile time.
Perhaps there is another way to test for decision tree explosion, but I'm not sure what that is.

@alfonsogarciacaro
Copy link
Member Author

It's not only the decision tree explosion, the code also gave me a chance to fix other typing issues (like casting non-ident expressions when accessing fields). But it's true it should be part of the integration tests. Although mocha has a timeout when running tests, maybe it's hit when the file takes too long to load. For now I've rebased the commit to add a simple assertion to make sure the code works.

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.

2 participants