Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to TypeScript? #376

Closed
7 tasks done
stasm opened this issue May 28, 2019 · 21 comments
Closed
7 tasks done

Migrate to TypeScript? #376

stasm opened this issue May 28, 2019 · 21 comments

Comments

@stasm
Copy link
Contributor

stasm commented May 28, 2019

Over the last month I've used TypeScript in a side-project, and it's been a delight. It's quick to pick up, doesn't get into the way, has excellent editor support in VS Code, and compiles to readable JS. I'd like to kick off a discussion about migrating the fluent.js family of packages to TypeScript,. We could start with the more user-facing ones: fluent, fluent-react, and fluent-langneg, which also have the benefit of being small, well tested, and of already having user-contributed typings thanks to @huy-nguyen.

See also: #329, #231.

Update (January 2020)

The migration is currently ongoing. I'll keep the following list up to date as more packages are migrated.

@huy-nguyen
Copy link

I'm willing to help out if need be.

@Pike
Copy link
Contributor

Pike commented Jun 3, 2019

FWIW, VS Code might be the only acceptable editor for typescript. I tried something small in Atom over the weekend, and it was very Rust, with the recommended add-on. No help, bustages on compile, unhelpful error messages. In VS Code, the experience was pretty different. But even then, there were warnings arount tslint that couldn't be fixed following the instructions.

I'd assume that typescript comes with quite a vendor lock in.

@Gregoor
Copy link
Contributor

Gregoor commented Jun 24, 2019

IntelliJ also works great for TS. Afaik lots of TS IDE functionality comes from its language server, so it should behave pretty similar between editors.
Though I'd agree that the error messages could be better, they can be pretty verbose.

@Seally
Copy link

Seally commented Jul 5, 2019

I'd be happy if this project moves to TypeScript as well. I'm thankful the TypeScript definitions exist (and it is usable), but I feel it's not quite "there" yet and there might not be enough eyes looking at it to keep up with all the changes and edge cases.

As for editors, Atom seems to have the atom-typescript plugin, but I don't know how the experience there compares to VS Code and Intellij/Webstorm.

@birtles
Copy link

birtles commented Sep 11, 2019

Not wanting to derail this issue into an editor discussion, but I write a lot of TS in vim using tsuquyomi for TS support--and there are other vim/TS projects too like nvim-typescript. I rarely open VS Code so I don't think vendor lock-in should be a concern for switching to TS.

@Mossop
Copy link

Mossop commented Oct 11, 2019

I would say I've also had great experiences using typescript in my projects and not having type definitions for fluent actively hampers my ability to use it so either this or #329 would be a big help.

I came to mention though that having proper types might end up changing some of the APIs so it isn't necessarily a backwards compatible change. The example I just hit is the Localized react element. As props it accepts well defined properties of id, attrs and children. But it also accepts any number of arguments as properties. As far as I can see TypeScript doesn't let you define an interface that contains both arbitrary properties and fixed properties unless all the fixed properties are of a type that is a subset of the type of the arbritrary properties. It would work to instead use a arguments: { [name: string]: ... } prop, but that is a change in the interface.

@stasm
Copy link
Contributor Author

stasm commented Nov 26, 2019

Good points about the API changes in fluent-react (and perhaps elsewhere). I think it was a mistake on my part to use implicit props like $name and link={<a href=".."/>}, and in fact I'd prefer a more explicit API with variables/arguments and elements. I understand it adds friction to the migration, however.

For now, I've started an experiment and ported @fluent/bundle to TypeScript on a branch: master...stasm:ts. Adding types was fairly straightforward, the API remains the same, and type declarations are generated correctly.

I'm having troubles with tests, however. I transpile TS files to ES modules but our test runner, mocha, doesn't support ES modules yet. I'll look for solutions and also for alternatives (like Jest).

@huy-nguyen
Copy link

huy-nguyen commented Nov 26, 2019

@stasm Jest works very well with TypeScript and also gives you code coverage out of the box. I think you should give it a try.

@stasm
Copy link
Contributor Author

stasm commented Nov 27, 2019

It does look like a very nice test runner indeed. Re. TypeScript, I think it supports it via transpiling input files with Babel, doesn't it? Which we could also do with mocha, I think, if needed. And since we would build ES code with tsc anyways, it would be nice to just use that for testing.

@Seally
Copy link

Seally commented Nov 27, 2019

I use it with ts-jest, which should make it go through tsc if I'm understanding it correctly. When I was using Mocha, I combined it with ts-node/register. I did this mainly because I write most tests in TypeScript though.

@stasm
Copy link
Contributor Author

stasm commented Nov 27, 2019

My first thought was to only migrate src and leave tests in JS, but maybe that's actually more work than just migrating everything at once. Thanks for the pointers to ts-jest and ts-node/register; I'll check them out.

@huy-nguyen
Copy link

huy-nguyen commented Nov 27, 2019

You don't need ts-jest anymore because Babel, which is already used by jest to handle ES modules, can transpile TypeScript. However, I don't think Jest + Babel does type checking but you most likely already do that as part of your development/production workflow. You don't have to migrate your tests to TypeScript. I usually write application code in TypeScript and test code in JavaScript.

@Seally
Copy link

Seally commented Nov 27, 2019

Small note that I have encountered a library written in TypeScript with tests written mostly in JavaScript that produced incorrect type definitions. The code worked fine in JS land, but it produced a compilation failure in TS. The bug was in an interface definition, so it didn't affect the JavaScript code behavior. Since all the tests for that module was in JS, type definitions weren't covered, and it apparently went undetected for quite some time.

With that said, the case seems to be fairly uncommon, and I agree that we probably wouldn't need to port all the tests to TypeScript.

@huy-nguyen
Copy link

huy-nguyen commented Nov 27, 2019

@Seally I'm not sure I follow your train of thoughts. Unless you make TS typecheck JS files (usually through checkJS: true in tsconfig.json), I don't see how the JS test code can cause the TS application code to produce type errors. Most likely the TS code already contains those type errors to begin with and would be caught by TS compilation regardless of whether the tests exist or written in JS.

@Seally
Copy link

Seally commented Nov 27, 2019

@huy-nguyen I'm not saying JavaScript test code causes TypeScript compilation errors, but that having test code written exclusively in JS means that the type definitions aren't covered by tests. This could possibly allow type definition issues to slip through if no one happened to try to import and use the code in TypeScript.

Although TS does help in avoiding a large class of bugs, and static typing is amazing for IDEs, it unfortunately also introduces a new class of bugs in its place.

For the case in question, it didn't cause compilation errors when the library was built because the type error could only be triggered when code was referenced, not when it was defined. As it was part of the public API and no other part depended on it, it was not referenced elsewhere within the project from TypeScript code.

The only code that referenced the exports in that project were the test cases, written in JS. They tested the runtime behavior of the code, which was correct. Since JS tests aren't type-checked, they did not test if it would compile when it was imported in TypeScript code and consumed.

The exact issue involved JSX typings (the project in question is inferno-redux) and it occurred when using JSX that referenced in that particular component. If I recall correctly, the issue was that the interface for the component's props had the wrong types defined for children. Adding child components to that component caused a compilation error. Not adding child components to it also led to a compilation error, for different reasons.

@huy-nguyen
Copy link

huy-nguyen commented Nov 28, 2019

@Seally I see what you mean. I think running tsc —noEmit will usually type check all TS files that are not in node_modules, regardless of whether they are referenced or not. Is that true in your case?

@Seally
Copy link

Seally commented Nov 28, 2019

I believe the aforementioned issue will show up only if I actually use the component in another program, since it wasn't caught during build which did go through tsc. noEmit, as I understand it, can only catch errors encountered during the build so that we can separate type-checking from compilation.

The problem in question, however, was a bit like this, rather horrible, example:

my-component source:

// This file compiles.
// ... required imports
export interface BadProps { 
    children?: VNode // In ComponentProps, this is defined as children?: InfernoNode
}
export class BadComponent extends Component { constructor(props: BadProps) { /* ... */ } }

Imported:

// ... some other imports to make this work
import { BadComponent } from "my-component"; // if we end the file here, this will still work.

render(<BadComponent>/* ... */</BadComponent>, /* root */); // type error

In this case, there's no valid JSX input that can go into the /* ... */ portion because children does not accept types like JSX.Element (VNode itself is an interface, while InfernoNode is a type alias for a union of types). This goes through TypeScript's initial type-checking on the library end since, during build, TS just saw it like every other class and compiled it without issues.

This is admittedly a niche case of course, and it was caused partly because some of the typings weren't strict enough. BadProps didn't have to extend from ComponentProps, so it could define children differently from what it was in ComponentProps.

With that said, I agree that TypeScript for test cases generally won't be needed. Additionally, JS test code can reach code paths that are usually restricted under TS. For tests dealing with potentially complicated typings (React hooks and Fluent resource bundle come to mind) however, it may be a good idea to write TS test cases with type-checking somewhere in the test pipeline just to be sure it won't cause issues when a TypeScript user imports and uses it.

@stasm
Copy link
Contributor Author

stasm commented Dec 2, 2019

I submitted #436 with my proof-of-concept port of @fluent/bundle.

I'd like to thank everyone who has contributed to Fluent's typings in the DefinitelyTyped repository. First of all, it helps people use Fluent in TS projects today, and it also helped me cross-check the types in my experiment :)

@stasm
Copy link
Contributor Author

stasm commented Jan 23, 2020

I just merged #436 and published @fluent/bundle 0.15.0, which is now written in TypeScript!

@stasm
Copy link
Contributor Author

stasm commented Jan 23, 2020

I'll keep this issue open to track progress of migrating the remaining @fluent/ packages to TypeScript.

@stasm
Copy link
Contributor Author

stasm commented Apr 14, 2020

As of today, all packages in this repository have been migrated to TypeScript, and for most of them, I published new releases to npm. Here's the full list:

Thanks to @Gregoor, @huy-nguyen, @Pike, @Seally, and @zbraniecki for the guidance, help and reviews!

@stasm stasm closed this as completed Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants