fluent.js icon indicating copy to clipboard operation
fluent.js copied to clipboard

Generate types declarations

Open macabeus opened this issue 5 years ago • 7 comments

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

macabeus avatar Apr 05 '20 19:04 macabeus

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.

stasm avatar Apr 14 '20 13:04 stasm

@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.

macabeus avatar Apr 20 '20 01:04 macabeus

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 #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 #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!

stasm avatar Apr 22 '20 13:04 stasm

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 avatar Apr 22 '20 15:04 macabeus

@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.

macabeus avatar May 03 '20 21:05 macabeus

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!

stasm avatar May 04 '20 12:05 stasm

@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

macabeus avatar Jul 05 '20 16:07 macabeus