Migrate @fluent/bundle to TypeScript #436

stasm · 2019-12-02T15:38:41Z

See #376 for the discussion. As an experiment, I ported @fluent/bundle to TypeScript. Migrating the files in src/ was a breeze; I did run into a few troubles with tests and packaging, however.

Summary

Files in src/ are now written in TypeScript.
make compile compiles them into corresponding JS files in esm/.
- This also produces type definitions.
- tsc is a project-based compiler which compiles many files at once. While it's possible to run it on a file-by-file basis, it's slower to do so and some features of TS won't work (e.g. const enum). I added an empty file at esm/.compiled which is touched every time tsc runs to indicate to make that the compiled files are up to date compared to the source in src.
I kept all test files in JS.
- They import from the esm/ directory, which means that the project needs to be compiled before tests are run.
- make test takes care of that automatically.
The other option for running tests was to use a Babel plugin for stripping TypeScript syntax from files in src at the moment when the tests are run.
- My own preference (at least for now) was to introduce a two-step process of first compiling the source and then running the tests. The compilation can fail early due to typing errors which shortens the feedback loop.
- I also liked the idea of running tests on actual compiled files in the esm folder rather than using one tool to compile them for shipping (tsc) and another to run tests (@babel/preset-typescript).
I used typedoc to produce API docs.
I didn't touch other parts of the build pipeline.
- I considered using tsc's target: "es2017" to produce the compat.js build, but in the end decided that it would be best to limit the number of moving parts in the PR.
- The compat.js build is still produced with Rollup and Babel, as it used to be.
- I filed Revisit Compatibility #435 to discuss a long term clean-up effort which could end up with us dropping Babel in favor of tsc's target: "es2017".

Work Remaining

I still need to configure the linter.

API Changes

I renamed the FluentType class to FluentBaseType, and then I re-introduced FluentType as a type alias in TS: type FluentType = string | FluentBaseType. I'm not sure about the naming, however, and I'm open to alternatives.
(Added on Dec 9) I removed FluentError. It was only used in the runtime parser. In the resolver we use native errors: RangeError, ReferenceError, and TypeError, so I changed all uses of FluentError to the native SyntaxError.

…tests

fluent-bundle/src/index.ts

fluent-bundle/src/builtins.ts

stasm · 2019-12-02T15:50:05Z

fluent-bundle/src/builtins.ts

+  }
+
+  if (arg instanceof FluentNumber) {
+    return new FluentDateTime(arg.valueOf(), { ...arg.opts, ...values(opts) });


FluentDateTime extends FluentNumber now, which made this possible. I needed a way to type-guard the access to .opts, which is only guaranteed to exist on FluentNumber and FluentDateTime. Initially I just made two instanceof checks here, but then decided that FluentNumber and FluentDateTime were close enough to make them related. FluentDateTime keeps its value as a number, too.

I'm having second thoughts about this.

In TypeScript, this works because Intl.DateTimeFormatOptions happens to be assignable to Intl.NumberFormatOptions. I'm not sure if we should rely on this.

On a higher level, this is a spec question: what should be the outcome of NUMBER($date) and DATETIME($number)? I think it will be safer to not make assumptions about the answer right now and wait for the formatting spec to be finished.

I'll revert the FluentDateTime extends FluentNumber change.

fluent-bundle/src/bundle.ts

stasm · 2019-12-03T09:00:02Z

@blushingpenguin, @huy-nguyen, @Seally -- would you be interested in taking a look at this PR and giving me hints about how to improve it? Thanks!

stasm · 2019-12-09T17:59:36Z

I got to a point where this PR is now ready for review. I'd like to flag both @Pike for a general review, and @zbraniecki, who has typing experience from working on fluent-rs.

I added explicit return types to all functions and methods and I like how it improves the reading experience when browsing the code, in particular outside the IDE. I have a few naming questions; I asked them in inline comments.

Pike · 2019-12-10T18:59:36Z

Some ad-hoc comments from the first phase of my review:

I tried to map the syntax ebnf to ast.ts, and struggled for two reasons:

the ordering of types and variants is different
the Runtime prefix dominates my ability to discover concepts I know.

The latter might have been exaggerated by the fact that I was looking at expressions, which means everything is called RuntimeFooExpression, which doesn't blend well with how humans read.

Or at least I read that they read by "first few characters and last few characters and sum of characters inbetween".

Pike

stas and I talked about this a bit, keeping track of this in the PR:

revert the behavior changes of NUMBER and friends on unused positional arguments
use plain Node names for classes in ast.js, as there's no ambiguity with @fluent/syntax

stas also suggested to rename the resolver functions from MessageReference to something that actually resembles what the function does. I'm not sure if that makes this patch easier or harder to review, but the general idea makes sense to me.

Gregoor

Only had time for a quick check, will look at it more later this week. Looks great so far, excited for a typed future! 🎉

fluent-bundle/src/resolver.ts

Gregoor · 2019-12-17T14:19:27Z

fluent-bundle/src/resolver.ts

  if (!func) {
-    scope.reportError(new ReferenceError(`Unknown function: ${name}()`));
-    return new FluentNone(`${name}()`);
+    switch (name) {


I think I liked the old way better, but I guess the way TS does exports it's not as easy to achieve?

Yeah, I was getting an error about typeof import(...) not having an index signature. A workaround which I found was to use let func = scope.bundle._functions[name] || (<any>builtins)[name];, which is shorter and much closer to the old way. Do you think it's a better choice here than the explicit switch?

... or add NUMBER and DATE to _functions in the bundle constructor?

Yeah, I was getting an error about typeof import(...) not having an index signature. A workaround which I found was to use let func = scope.bundle._functions[name] || (<any>builtins)[name];, which is shorter and much closer to the old way. Do you think it's a better choice here than the explicit switch?

Yeah no I guess I agree that the switch is better than that, because we don't lose type info that way. Another option which imo beats switch would be sth like:

let func = scope.bundle._functions[name] || ( name == "DATETIME" || name == "NUMBER" ? builtins[name] : null );

There should also be a more generic version to achieve the type refinement necessary to use name as an index, but I'm not coming up with it rn 🤔

I like @Pike's suggestion to deal with this when the bundle is constructed rather than later on, when translations are resolved. I implemented it in 4d48ced.

stasm · 2020-01-13T17:30:38Z

I updated this PR to merge cleanly into the current master.

Pike

The ast is much more readable now, which has the side effect that I found a few more things.

General things I still need to learn is what TS types actually do, in terms of instanceof. I wonder if we could get rid of the pseudo type type.

Also, I'd like to learn more about the use of public.

fluent-bundle/src/ast.ts

Pike · 2020-01-15T13:03:20Z

fluent-bundle/src/resolver.ts

  if (!func) {
-    scope.reportError(new ReferenceError(`Unknown function: ${name}()`));
-    return new FluentNone(`${name}()`);
+    switch (name) {


... or add NUMBER and DATE to _functions in the bundle constructor?

fluent-bundle/src/resource.ts

Pike

One nit question, one question more about understanding TS, but this looks good to me either way.

fluent-bundle/src/bundle.ts

stasm · 2020-01-23T10:06:38Z

I have verified that @fluent/bundle built from TypeScript works correctly in a React app (The Fluent Playground) and in Firefox (as Fluent.jsm).

stasm added 25 commits November 21, 2019 11:46

WIP Rewrite to TypeScript

5a3ba52

Build files from esm/

b0fb0b0

Remove default exports

0c7adcd

Type custom functions

dd1b458

FluentType, FluentBaseType

5a44fce

RuntimePattern, RuntimeComplexPattern

3b010e9

Args are unknown

2093617

Remove RuntimeInlineExpression type

242acc1

Simplify the RuntimeMessage type

a804eba

RuntimeTerm

3a5bf23

CustomFunction => FluentType

78a1aed

FluentBaseType<T>

cbff094

FluentArgument

a661351

Emit declarations

8c7650f

Use .js in imports

1536a4b

Fix types in formatPattern

a1b4479

FluentDateTime extends FluentNumber

3517b5a

Test compiled ESM files

22c0298

make compile, make test

32a0788

noImplicitAny is implied by strict: true

847273e

Make @fluent/dom, @fluent/react depend on @fluent/bundle for running …

7811060

…tests

Generate API docs with typedoc

cbb7ceb

Update .npmignore

2b958e8

Remove esdoc.json

2a88b88

Add types to package.json

86ce29b

stasm mentioned this pull request Dec 2, 2019

Migrate to TypeScript? #376

Closed

7 tasks

stasm commented Dec 2, 2019

View reviewed changes

stasm added 2 commits December 2, 2019 17:10

Fix fluent-gecko's Fluent.jsm

b9fad68

Type Scope.memoizeIntlObject

6389bac

stasm marked this pull request as ready for review December 9, 2019 17:50

stasm requested review from zbraniecki and Pike December 9, 2019 17:59

Pike suggested changes Dec 17, 2019

View reviewed changes

Gregoor reviewed Dec 17, 2019

View reviewed changes

Seally mentioned this pull request Dec 18, 2019

[fluent] Upstream renamed packages DefinitelyTyped/DefinitelyTyped#37541

Closed

4 tasks

stasm added 7 commits December 18, 2019 13:15

Call resolver functions more consistently

83c0c99

Drop the Runtime prefix from AST types

dbe37dd

Remove the arity check from builtins

cabdb9a

Rename CustomFunction to FluentFunction

6128600

Rename CustomTransform to FluentTransform

189a996

Merge branch 'master' into ts-bundle

fa2b5f3

Merge branch 'master' into ts-bundle

f1f69a3

Pike reviewed Jan 15, 2020

View reviewed changes

stasm added 6 commits January 15, 2020 16:25

args: Array<Expression | NamedArgument>;

3eeb319

Move Indent into resource.ts; make it a class

af4d3b4

Remove FluentResource._parse

bd5ba8a

export type FluentValue = FluentType<unknown> | string;

03ea7a0

_functions = {NUMBER, DATETIME, ...functions}

4d48ced

Export FluentType in fluent-gecko

cad6b9b

stasm requested a review from Pike January 15, 2020 16:49

Pike approved these changes Jan 15, 2020

View reviewed changes

fluent-bundle/src/bundle.ts Outdated Show resolved Hide resolved

fluent-bundle/src/bundle.ts Show resolved Hide resolved

Clarify the type definition of FluentArgument

3218d81

stasm mentioned this pull request Jan 16, 2020

Use RegExp constructor instead of using literal expression #444

Open

stasm merged commit e992e23 into projectfluent:master Jan 23, 2020

stasm deleted the ts-bundle branch January 23, 2020 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate @fluent/bundle to TypeScript #436

Migrate @fluent/bundle to TypeScript #436

stasm commented Dec 2, 2019 •

edited

Loading

stasm Dec 2, 2019

stasm Dec 3, 2019

stasm commented Dec 3, 2019

stasm commented Dec 9, 2019

Pike commented Dec 10, 2019

Pike left a comment

Gregoor left a comment

Gregoor Dec 17, 2019

stasm Dec 18, 2019

Pike Jan 15, 2020

Gregoor Jan 15, 2020

stasm Jan 15, 2020

stasm commented Jan 13, 2020

Pike left a comment

Pike Jan 15, 2020

Pike left a comment

stasm commented Jan 23, 2020

Migrate @fluent/bundle to TypeScript #436

Migrate @fluent/bundle to TypeScript #436

Conversation

stasm commented Dec 2, 2019 • edited Loading

Summary

Work Remaining

API Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm commented Dec 3, 2019

stasm commented Dec 9, 2019

Pike commented Dec 10, 2019

Pike left a comment

Choose a reason for hiding this comment

Gregoor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm commented Jan 13, 2020

Pike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pike left a comment

Choose a reason for hiding this comment

stasm commented Jan 23, 2020

stasm commented Dec 2, 2019 •

edited

Loading