Typing fixes for Pontoon TypeScript migration, fix #524 #525
Conversation
Pike
commented
Mar 17, 2021
Pike
commented
- Stricter typing for @fluent/langneg
The AST now aligns with the fluent-rs one, and less so with the reference AST. That's OK as it helps consumers of the library in types JS variants to use narrowing reliably.
Pike
force-pushed
the
strict-typing
branch
3 times, most recently
from
7c20668 to
8dab470
Compare
| getInlineExpression(ps: FluentParserStream): AST.Expression | AST.Placeable { | ||
| getInlineExpression( | ||
| ps: FluentParserStream | ||
| ): AST.InlineExpression | AST.Placeable { |
There was a problem hiding this comment.
suggestion (if-minor): this could be a bit cleaner if we separated getPlaceable from getInlineExpression.
There was a problem hiding this comment.
I gave this a look, and my intuition isn't strong here. And I don't think our tests are exhaustive. Like, I was rather surprised that
shared-photos = {NUMBER({1 ->
*[1] 2
})}
works. Which made me wonder if it's not about expressions when it comes down to block vs inline, but placeable.
I've paged most of that context out of my brain, so it's not minor to me.
Playground also seems to behave differently to my local repo, and I'll call shotgun on that one. It still uses the 0.14 compat build.
There was a problem hiding this comment.
ouch. That's not spec compliant, right?
Spec doesn't allow for it - https://github.com/projectfluent/fluent/blob/master/spec/fluent.ebnf#L74
Rust doesn't allow for it - https://github.com/projectfluent/fluent-rs/blob/master/fluent-syntax/src/parser/expression.rs#L176
maybe it's time to consider Rust->JS export for the parser?
There was a problem hiding this comment.
I think the spec allows it, https://github.com/projectfluent/fluent/blob/master/spec/fluent.ebnf#L59 has the inline_placeable as inline expression. And I tested the fluent-rs parser, too, and it was fine.
jazz = {{{1->
*[1] 2
}}->
*[1] 3
}
OTH is illegal. Works on playground as in not only in the resolver, but also shows an AST.
There was a problem hiding this comment.
ah, right. The placeholder loophole in inline :) retracting my previous statement.
|
I've also tested this with temporarily installing my branch on the corresponding Pontoon PR, mozilla/pontoon#1902. When doing the TS conversion there, I end up with just two remaining issues, where the Transformer claims to not have the Which is a long winding way of saying that this doesn't just look nice in theory, but also stood the test of a TypeScript consumer. |
