(@fluent/syntax) Migrate to TypeScript#457
Conversation
|
@Pike, here's the finished PR migrating |
Pike
left a comment
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
What does this do? Once more mostly my curiousity.
There was a problem hiding this comment.
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.
|
Thanks for the review, @Pike!
TS doesn't touch thte
Good question.
Yeah, I think so.
Good idea, I'll reach out. |
|
I updated the PR to address your comments. I'd like to look again at the index signatures of |
|
I relaxed the index signature, documented both the |
Pike
left a comment
There was a problem hiding this comment.
LGTM. Giving an r+ assuming that the Pontoonies survive ;-)
|
I've verified in a local instance that |
See #376.