Skip to content

(@fluent/syntax) Migrate to TypeScript#457

Merged
stasm merged 9 commits intoprojectfluent:masterfrom
stasm:ts-syntax
Mar 2, 2020
Merged

(@fluent/syntax) Migrate to TypeScript#457
stasm merged 9 commits intoprojectfluent:masterfrom
stasm:ts-syntax

Conversation

@stasm
Copy link
Copy Markdown
Contributor

@stasm stasm commented Feb 25, 2020

See #376.

@stasm stasm mentioned this pull request Feb 25, 2020
7 tasks
@stasm stasm requested a review from Pike February 25, 2020 12:34
@stasm
Copy link
Copy Markdown
Contributor Author

stasm commented Feb 25, 2020

@Pike, here's the finished PR migrating @fluent/syntax to TypeScript. Would you have a moment to take a look at it? I expect fluent-syntax/src/ast.ts will be the most interesting, but I'm looking for general feedback too. Thanks!

Copy link
Copy Markdown
Contributor

@Pike Pike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally this looks good, a couple of questions and nits, though.

One thing I only realized in this patch, is the use of .ts files, but imports from .js files. That puzzled me, though VS Code doesn't have a problem resolving those.

Generally, does the introduction of [].includes change the browser compat matrix? Asking for Pontoon.

And there are a few more left-over checks on node.type, I wonder if you want to convert them to instanceof checks, too?

And then maybe this one would be good to go through a feedback loop of a Pontoony, I just noticed that I don't know how they construct Fluent instances in the rich editor.

Also a few nits/questions inline.

constructor() {}
export abstract class BaseNode {
public type = "BaseNode";
[name: string]: unknown;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do? Once more mostly my curiousity.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an index signature. The Visitor needs it because it dynamically accesses methods based on the type of the visited node. I've used unknown so that the subclasses of BaseNode can narrow down the types of their members according to their needs.

@stasm
Copy link
Copy Markdown
Contributor Author

stasm commented Feb 26, 2020

Thanks for the review, @Pike!

One thing I only realized in this patch, is the use of .ts files, but imports from .js files. That puzzled me, though VS Code doesn't have a problem resolving those.

TS doesn't touch thte from part of imports. Including .js makes the generated output (in esm/) work in <script type=module> situations. Node as well as Rollup would also accept extensionless paths, which is what most JS/TS projects generally do. FWIW I think this is a bad practice because it precludes browser compatibility from the get go.

Generally, does the introduction of [].includes change the browser compat matrix? Asking for Pontoon.

Good question. Array.prototype.includes was added in Firefox 43 (we support 52+) and Chrome 47 (we support 55+). Old Edge and Safari should also work.

And there are a few more left-over checks on node.type, I wonder if you want to convert them to instanceof checks, too?

Yeah, I think so. instanceof helps TS infer types, which improves the quality of the code and the editing experience.

And then maybe this one would be good to go through a feedback loop of a Pontoony, I just noticed that I don't know how they construct Fluent instances in the rich editor.

Good idea, I'll reach out.

@stasm
Copy link
Copy Markdown
Contributor Author

stasm commented Feb 26, 2020

I updated the PR to address your comments. I'd like to look again at the index signatures of Visitor and Transformer. I think the way I declare them right now would make it hard for consumers using TypeScript to keep state on the instances of subclasses. I'll fix this tomorrow.

@stasm stasm requested a review from Pike February 27, 2020 12:48
@stasm
Copy link
Copy Markdown
Contributor Author

stasm commented Feb 27, 2020

I relaxed the index signature, documented both the Visitor and the Transformer, and also explicitly listed all possible visit${node} methods to help implementors get their type right.

Copy link
Copy Markdown
Contributor

@Pike Pike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Giving an r+ assuming that the Pontoonies survive ;-)

@stasm
Copy link
Copy Markdown
Contributor Author

stasm commented Feb 28, 2020

I've verified in a local instance that @fluent/syntax built from this PR works in Pontoon. I'll merge and release on Monday.

@stasm stasm merged commit 2b84fbe into projectfluent:master Mar 2, 2020
@stasm stasm deleted the ts-syntax branch March 2, 2020 13:27
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