Skip to content

Generate types declarations #466

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

Open
macabeus opened this issue Apr 5, 2020 · 7 comments
Open

Generate types declarations #466

macabeus opened this issue Apr 5, 2020 · 7 comments

Comments

@macabeus
Copy link
Contributor

macabeus commented Apr 5, 2020

Currently we could define messages with variables:

hello = Hello { $name }
age = Your age is { $age }

and generating a bundle, we could use that on our code:

let hello = bundle.getMessage("hello")
console.log(bundle.formatPattern(hello.value, {name: "Anna"}))

let age = bundle.getMessage("age")
console.log(bundle.formatPattern(age.value, {age: "23"}))

But since there are lacking type informations, TS can't check if I'm writing all variables that message requires, or if I typed something wrong, as well as it's missing autocomplete.

bundle.formatPattern(hello.value); // I forgot to set the name
bundle.formatPattern(hello.value, {nam: 'Anna'}); // typo
bundle.formatPattern(hello.value, {age: 23}); // very bad!

We could improve even more if we could read the comments

# $name (String) - The user name
hello = Hello { $name }
bundle.formatPattern(hello.value, {name: 23}); // type error: should be string

On this small example, a valid type declaration would be:

type MessagesKey = 'welcome' | 'age'

type PatternArguments<T extends MessagesKey> = (
    T extends 'welcome'
        ? { name: string }
        :
    T extends 'age'
        ? { age: number }
        : never
)

export declare type Message<T> = {
    id: T;
    value: Pattern<T> | null;
    attributes: Record<string, Pattern<T>>;
};

export declare class FluentBundle {
  // inside of FluentBundle...

  getMessage<T extends MessagesKey>(id: T): Message<T>;
  formatPattern<T extends MessagesKey>(pattern: Pattern<T>, args?: PatternArguments<T>, errors?: Array<Error> | null): string;
}

image

@stasm
Copy link
Contributor

stasm commented Apr 14, 2020

This is a really interesting idea! I think it would improve the developer experience in TypeScript by a lot.

It would be interesting to see a proof-of-concept prototype of this. If you're up for it, I think the best place to start would be @fluent/syntax's Visitor class.

@macabeus
Copy link
Contributor Author

macabeus commented Apr 20, 2020

@stasm Hey, I just launched the first proof of concept version: https://github.com/macabeus/fluent-typescript-loader 🎉
I would be happy if you could check that =]

At this moment, I'm generating the .d.ts file for just one .ftl.
I'll have more work to could generate correctly the types on projects that has more than one .ftl file, because I'll need to merge that on just one file.
Also we'll need to check if it'll work with fluent-react.

Anyway, fluent-typescript-loader already is working on the most simple case.


We would improve types such as { name: string | number } to be { name: string } when we add semantic comments on Fluent.

@stasm
Copy link
Contributor

stasm commented Apr 22, 2020

Very exciting, @macabeus! I can't wait to try this out in a project!

I looked at the implementation, and I'd like to provide some high level feedback:

  • I would encourage you to not use @fluent/bundle's AST. It's meant to be internal and is optimized for the memory and performance requirements of the runtime. It might change in the future without notice. See the Internal representation of Pattern is private point in the @fluent/bundle 0.14 changelog.

    Instead, you should be able to use @fluent/syntax for this, which provides a convenient FluentParser.parse API, and has a well-defined AST. It's the same package that powers the AST tab in the Fluent Playground.

    @fluent/syntax also provides a Visitor API which you could use to walk the AST of a message and collect all variable names referenced in it. You'd just need to implement a visitVariableReference method on a subclass of the visitor. See this test in Gecko for a real-life usage example of this API.

  • Related to the above: may I suggest to not import directly from the esm folder?

    import { ComplexPattern } from '@fluent/bundle/esm/ast';
    

    As mentioned above, ComplexPattern isn't meant to be part of the public API. Furthermore, in Index.cjs #480 I'm experimenting with Node's conditional exports. If successful, it will make it impossible to import from specifiers inside the package.

  • Rather than string | number, you could declare the arguments as FluentArgument.

    Side note: I might rename FluentArgument to FluentVariable in Rename FluentArgument to FluentVariable #481. I'll ping you if I do.

    Also, +1 to the comment about semantic comments.

  • Lastly, I really like how you used the id as a type parameter, which made it possible to type the return value of getMessage as the generic Message<T>, which means formatPattern can know about whihc message the pattern came from. That's neat.

I hope this help. Let me know if you have any questions!

@macabeus
Copy link
Contributor Author

Very thank you for the review.
I'll update the package following your suggestions, as well as change to use chokidar (or watchman?) instead of webpack loader, so we'll could use it without webpack.

@macabeus
Copy link
Contributor Author

macabeus commented May 3, 2020

@stasm Hey, I just released a new version: https://github.com/macabeus/fluent-typescript/releases/tag/0.0.3 ! 🎉

Now I think that this tool is so much more stable.
Could you take a review on codebase again, please?

Now I want to add support to fluent-react and react-i18next.

@stasm
Copy link
Contributor

stasm commented May 4, 2020

Nice work, @macabeus! I installed it in a test project, and I was impressed by the results!

It looks like the project is maturing nicely. I have two pieces of feedback, but overall it looks great. I hope more people will start using it!

  • I didn't understand at first that fluent-typescript starts a watcher. I ran it, it said Ready, and... nothing happened :) I had to edit one of the Fluent files in my project for it to do its things. Perhaps consider adding a non-watcher mode to the CLI command?

  • Good work implementing a Visitor. As a slight improvment, rather than having two visitor classes you could try having a single class with both visitMessage and visitVariableReference methods. In visitMessage you could set this.currentMessageId and this.currentVariableReferences = new Set();, and then call this.genericVisit() to descend into the message, including any references in its patterns. But like I said, this would be just a small improvement; no need to do it if you prefer the current approach.

Nice work!

@macabeus
Copy link
Contributor Author

macabeus commented Jul 5, 2020

@stasm I was very busy on the last couple of weeks, but finally I added support to @fluent/react as well as react-i18next, and I added auto-emit when start to watch.
Now it's a little more useful CLI tool to use with Fluent =]

https://github.com/macabeus/fluent-typescript/releases/tag/0.0.4

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

No branches or pull requests

2 participants