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

[RFC] Migrate Jest to TypeScript #7554

Closed
wants to merge 3 commits into from

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Dec 27, 2018

In the lastest State of JS, the one stat that stood out to me was the number of people either using or wanting to use TypeScript (over 46.7% has used and will use it again, 33.7% wants to learn it). And since Babel 7 has support for TypeScript, integrating it into your workflow has become way easier than it used to be.

We, the Jest core team, have privately discussed migrating to TypeScript (Jest is currently written using Flow), and I was asked to look into how much effort it potentially is, and try to gather feedback from the community. So, as a proof of concept, I've migrated pretty-format over (look at the second commit of this PR).

This PR is a request for comments from the community. Is it a good idea? Is it wasted effort?
While comments on the code itself (and the build process) is welcome, that's not really the point of this particular exercise.

The PR will probably not pass CI (at least linting), but that should be fixed by babel/babel-eslint#711.


From my personal (and limited) experience, the two type systems are pretty similar in feature sets and syntax, so I haven't consider which of the 2 type systems are better. This is about improving the health and maintability of the code base and encouraging contributions.
I'll try to list out the pros and cons of going Flow -> TypeScript from the top of my head - not by any means an exhaustive list!

Pros:

  • Better tooling (IDEs in particular)
  • Way more typings available for community modules, meaning our own code is safer
  • We can easily distribute types for both Jest itself (@types/jest has 1M download every week) and for Jest's individual packages, such as jest-worker and jest-haste-map
  • More popular, which hopefully would lower the barrier of entry for contributions from the community

Cons:

  • Migration cost (both time spent on the migration itself, and the churn imposed on existing PRs)

Other libraries doing the same:

I'm sure one can find evidence of going the other way as well, those are just the ones I know about, and presented as anecdotal "evidence".

@TrySound
Copy link
Contributor

For me it just doesn't make sense. You will invest a lot of time and will get a bit better autocomplete (I know you may get more features). But in the end of the day it's the same javascript full of its problems.

For me I see the sense in moving to reason or ocaml which are more safe languages than both flow and typescript.

@orta orta requested a review from cpojer December 27, 2018 14:49
@orta
Copy link
Member

orta commented Dec 27, 2018

For me I see the sense in moving to reason or ocaml

I think that's probably a step too far in terms of time + commitment for value. I think it would make a lot of sense for a reason testing library to take a bunch of the best ideas from jest and re-apply them in their own context.

Personally, I've been a part of migrating all of Artsy's JavaScript to TypeScript, and while it's a little bit of churn at first (notably the .js -> .tskills git history, long PRs need a harder rebase, etc) it;'s paid off in terms of dev tooling across the board

While I'll be sad to see my main test-bed for the flow for vscode extension go away, it'll be great for exploring and understanding the jest codebase (both as an occasional contributor, and definitely as someone who uses the sub-packages)

@jamesisaac
Copy link

Better tooling (IDEs in particular)

Flow has recently been putting lots of effort into rolling out support for Microsoft's Language Server Protocol (search "LSP"), which from waht I understand should allow it to provide the same level of integration into most IDEs that TypeScript does.

Way more typings available for community modules, meaning our own code is safer

Not sure "safer" is the right word here... in my experience Flow typings are generally more sound and expressive than their TS counterparts. Agreed there are certainly more definitions available though.

We can easily distribute types for both Jest itself (@types/jest has 1M download every week) and for Jest's individual packages, such as jest-worker and jest-haste-map

Not sure I understand this point - what's the difference from Flow & flow-typed?

More popular, which hopefully would lower the barrier of entry for contributions from the community

Imo jumping between TS and Flow does not create much of a barrier for someone experienced with either. Like you say, the features and syntax are extremely similar.


Another con:

  • Flow and Jest are both Facebook products (right?), so I assume (a) the barrier to entry for internal FB employees would be higher, and (b) if changes are needed upstream in the typechecker, the chances of that being prioritised are higher than with TS.

@SimenB
Copy link
Member Author

SimenB commented Dec 27, 2018

For me I see the sense in moving to reason or ocaml which are more safe languages than both flow and typescript.

@orta already answered here, but the tl;dr (at least to me) is that the point here is to make the code base more approachable to to new (and old) contributors who are consumers of Jest. The vast majority of them are JavaScript developers, not OCaml developers.

But in the end of the day it's the same javascript full of its problems.

True, but we love it nonetheless 🙂


Flow has recently been putting lots of effort into rolling out support for Microsoft's Language Server Protocol

Sure, but it's taken months and months, and it's still not done. The argument "we'll be just as good, just a year later" doesn't really spark much confidence.

Happy to be convinced otherwise of course.

Also, FWIW I personally don't use VS Code, so LSP doesn't impact my experience. I use IntelliJ (WebStorm), and their TS support is miles ahead of the Flow support. I know most people do use VS Code nowadays though, so your point is very much valid to the majority.

Not sure I understand this point - what's the difference from Flow & flow-typed?

There is no way to generate typings from source code in flow. They've had an untouched gen-flow-files for some time, but it never went anywhere (facebook/flow#5871)

Imo jumping between TS and Flow does not create much of a barrier for someone experienced with either. Like you say, the features and syntax are extremely similar.

Sure, but the tooling is way better with typescript, even if the syntax itself is very similar. The fact flow is improving (lsp support has been worked on for a long time, generating types etc) doesn't mask the fact TS has all of that today, and there's no real reason to believe Flow will get those features in a timely manner.

Flow and Jest are both Facebook products (right?), so I assume (a) the barrier to entry for internal FB employees would be higher, and (b) if changes are needed upstream in the typechecker, the chances of that being prioritised are higher than with TS.

I can't answer to a, but to b: not necessarily, this took 8 months facebook/flow#6103, typescript had it within a few days DefinitelyTyped/DefinitelyTyped#24624

@jeysal
Copy link
Contributor

jeysal commented Dec 27, 2018

Not sure "safer" is the right word here... in my experience Flow typings are generally more sound and expressive than their TS counterparts.

Note that TS has been getting a lot better since they started introducing the various strict checks. As long as you have strict: true (which Simen did enable in this proof of concept) and of course avoid any as much as possible, TS is almost equally safe nowadays. The main reasons why I still used Flow for some things a year ago were its more robust type system and the fact that it didn't need a dedicated compiler that is not integrated with Babel etc., both of which no longer apply.

@jamesisaac
Copy link

Sure, but it's taken months and months, and it's still not done. The argument "we'll be just as good, just a year later" doesn't really spark much confidence.

I feel like a lot of the IDE support actually is done now. The LSP mode just landed as default 9 days ago. In my experience, it works smoothly.

Also, FWIW I personally don't use VS Code, so LSP doesn't impact my experience. I use IntelliJ (WebStorm), and their TS support is miles ahead of the Flow support.

Hmm, does IntelliJ not have support for plugging in LSP? That seems a more scalable and robust approach than them writing in-house tooling for every language extension.

There is no way to generate typings from source code in flow. They've had an untouched gen-flow-files for some time, but it never went anywhere (facebook/flow#5871)

Agree this is a big pain point. Perhaps someone on the Jest team can help flag this up internally at FB and get help get this more attention?

I can't answer to a, but to b: not necessarily, this took 8 months facebook/flow#6103, typescript had it within a few days DefinitelyTyped/DefinitelyTyped#24624

Imo this just highlights the difference in philosophy between TS and Flow (which is healthy for the ecosystem). TS added a few lines of typedefs, which include introducing 4 any types. The Flow team held off on the early PRs as they were deemed unsound, and instead did a big internal refactor to better handle these new types.

Fundamentally I just think having the competition of two popular type systems is good for the JS ecosystem as a whole. So it's nice that there are some projects firmly planted on either side (Flow: FB products such as React, React Native, Jest, Relay, etc; TS: Microsoft products, Angular, etc). Remember that when Flow was first released, TS didn't even have support for null checks. The two have both been introducing new ideas, taking lessons from each other, and helping push the space forwards (TS from an OOP background encouraging pragmatism, and Flow from an algebraic data types background encouraging correctness). Imo it would be a shame for the ecosystem to become completely dominated by one type system, and lose the innovation and cross-pollination that the other is bringing to the table.

@corbinu
Copy link

corbinu commented Jan 2, 2019

I think this is a great idea! just a note Vuejs is also moving to TypeScript

@milesj
Copy link

milesj commented Jan 2, 2019

As a Flow to TypeScript convert, I will always agree with any such proposal. That being said, the time required to migrate an entire codebase is quite high, as it took me many months to convert all of my own projects, and the same at my employer. Doing it piece meal can also be quite confusing, especially if the conversion takes a lengthy amount of time (which it probably will).

@SimenB
Copy link
Member Author

SimenB commented Jan 9, 2019

@NoHomey @jwbay @asvetliakov @alexjoverm @epicallan @ikatyang @wsmd @JamieMason @douglasduteil @ahnpnl @JoshuaKGoldberg @UselessPickles @r3nya @Hotell @sebald @andys8

Sorry about the mass-tag (and some of you might already have responded to the OP) 🙏. As you're all set up as maintainers of @types/jest, I'd love to hear if you have any feedback here?

General feedback would be lovely (as well on thoughts about the process/transpiling part), but in particular I'm not sure if it makes sense for us (Jest core) to maintain the main Jest typings (for assertions etc) in this repo. It'd be coupled to Jest's release cycle, which is at times pretty slow. But at least all the types for transformers, config etc can come from here (a quick look through of @types/jest shows some holes in the various options). Maybe we can make the assertion typings good enough over time so that they don't change so often, though? Not sure, I've never tried to maintain typings before

@JM-Mendez
Copy link

@milesj did you use babel 7 as your transpiler during the migration? I recently completed migrating a flow-typed module to typescript, and using babel 7 with esmoduleInterop: true in the tsconfig I was able to have both files living side by side fairly easily. The only workaround was ignoring the error for flow-typed imports.

@milesj
Copy link

milesj commented Jan 11, 2019

@JM-Mendez On my personal stuff I used TS itself, while at work we used Babel 7. Definitely easier when using Babel.

@SimenB
Copy link
Member Author

SimenB commented Jan 15, 2019

We've decided to go for this, thanks a bunch for your feedback!

Right now, the plan is to release Jest 24 stable (should be any day now, https://github.com/facebook/jest/milestone/8 (please install jest@beta to test in your projects!)), then allow for a few weeks in case we need to make any bugfix releases.

After that, we want to do 2 things for Jest 25: simplify our configuration (#7185) and migrate to TypeScript.

As part of migrating to TS, we will be moving to ESM all over (tried to do it for Jest 24, but it broke lots of stuff, see #7602 and #7608). So we also need to make sure our exports do not mix default and named exports, so that we can use https://www.npmjs.com/package/babel-plugin-add-module-exports. That will also resolve #5803 and #7109. However, it'll likely be a breaking change, so putting it in a major makes sense.

I'll open up a tracking issue once we're ready to start migrating, at which point help from the community in actually migrating would be awesome 🙂 We'll hopefully be able to use https://github.com/bcherny/flow-to-typescript, which should remove a lot of the manual migration work.

@SimenB SimenB closed this Jan 15, 2019
@Jessidhia
Copy link

Jessidhia commented Jan 16, 2019

Not to turn this into a bikeshed, especially as the RFC has been accepted (:tada:), but I strongly recommend against the add-module-exports plugin. It either turns what would be future semver-minor changes (adding an export) into semver-major, or forces you to attach properties to what was your default export and now you're back to the same problem as commonjs exports with none of the advantages of ES module exports.


Also, make sure to enable --esModuleInterop with typescript. It's really common, especially in code samples you find on the internet, for code to erroneously import non-__esModule commonjs modules as if they were ES module namespaces. As far as I've seen in my little bubble, React in particular is guilty of that (import * as React from 'react', import { Component } from 'react', etc).

@SimenB
Copy link
Member Author

SimenB commented Jan 16, 2019

Thanks for chiming in @Kovensky!

I strongly recommend against the add-module-exports plugin

If we do not add it, I don't think we can use default exports (from our module entrypoints) at all. Forcing consumers to use .default is super ugly and kinda out of the question. Using just named exports is probably fine as well, except it'll force people who replace parts of Jest (e.g. the resolver) to do something like module.exports.resolver = resolver, which might feel unnatural. Not sure what the best tradeoff is, but const jestThing = require('jest-thing').default is not really an option. And the types generated by tsc doesn't understand module.exports when generating types, which is why I want to move to ES exports.

Also, make sure to enable --esModuleInterop with typescript.

We'll be using babel to transpile (and tsc for .d.ts files), so I don't think we need to do that? Or do I misunderstand that flag?

@milesj
Copy link

milesj commented Jan 16, 2019

Forcing consumers to use .default is super ugly and kinda out of the question.

Why not have a special index entry point that re-exports the default so consumers don't have to?

module.exports = require('./lib/index').default;

We'll be using babel to transpile (and tsc for .d.ts files), so I don't think we need to do that? Or do I misunderstand that flag?

Type checker (tsc) will still need the esModuleInterop, so it's best to just use it since that's the suggested path forward.

@rbuckton
Copy link
Contributor

Note that using add-module-exports will make it so that the typings in your .d.ts files will not reflect that actual export behavior of your code (i.e., you say export default foo in your TypeScript code, but its transpiled to module.exports = foo). This can cause problems when the TypeScript typings are used for JS consumers in editors like VS Code that use the TypeScript language service.

TypeScript has had a custom export = foo syntax for this case for a long time, which accurately reflects the actual export behavior of the transpiled code.

@SimenB
Copy link
Member Author

SimenB commented Jan 16, 2019

Why not have a special index entry point that re-exports the default so consumers don't have to?

That won't mess up the typing information? The main field wouldn't point to a file that has a *.d.ts sibling. Maybe we can use typings field in package.json to get around it?

I've just written apps in TS never a library to be consumed as both CJS and TS (and UMD), so I'm not sure how to ensure all of those cases work without having to care about how they're written. So all my questions are in good faith! 😀


This can cause problems when the TypeScript typings are used for JS consumers in editors like VS Code that use the TypeScript language service.

How so, the .default will also be there? I guess they won't be able to do will be forced to add .default to satisfy the types?

TypeScript has had a custom export = foo syntax for this case for a long time, which accurately reflects the actual export behavior of the transpiled code.

Hmm, for the docs (https://www.typescriptlang.org/docs/handbook/modules.html):

When exporting a module using export =, TypeScript-specific import module = require("module") must be used to import the module.

Doesn't seem like a feasible solution?

@ahnpnl
Copy link
Contributor

ahnpnl commented Jan 16, 2019

I've just written apps in TS never a library to be consumed as both CJS and TS (and UMD), so I'm not sure how to ensure all of those cases work without having to care about how they're written. So all my questions are in good faith! 😀

Maybe you can take a look at how ng-packagr builds packages ? I'm using it to create Angular packages. It helps to generate esm, fesm and umd as well.

@rbuckton
Copy link
Contributor

This was a problem with Chalk for a time. Their d.ts file indicates export default, however this caused issues with JS imports at design time. If you used Chalk in a JS file in VS Code (with // @ts-check), you would get errors if you just tried to use const chalk = require('chalk'); because the typings didn't agree with the transpiled output. They added https://github.com/chalk/chalk/blob/master/index.js#L213 so that they still have an actual default on the export (meaning you would still have to do const chalk = require('chalk').default to get the correct types).

I'm not expressly discouraging you from using add-module-exports, but would rather encourage you to ensure that the end-user experience is what you would expect. In the worst case, add-module-exports has an option to add a default, similar to what Chalk does.

@Andarist
Copy link
Contributor

Using add-module-exports might also BREAK users using webpack and other tools using "stricter" semantics - webpack/webpack#7973 (if you plan to use both main & module fields in this case).

Either way - declaring one shape (in the types, and possibly exporting it too from ESM entry) and having different in CJS/main entry might lead to some really obscure situations and bugs. While .default or require('lib').lib are ugly (I agree on that!) they have a superior advantage - they "just work" and are safe in all scenarios.

Unfortunately CJS & ESM interop is a mess 🤷‍♂️

@lll000111
Copy link

lll000111 commented Jan 16, 2019

Yesterday on HackerNews: "Porting 30K lines of code from Flow to TypeScript (davidgom.es)"

HN discussion: https://news.ycombinator.com/item?id=18906405

As a heavy Flow and occasional TS user, I found the writing not too long, balanced and informative, so it may be of interest in the current context.


PS: By the way, I came here from this HN discussion posted today.

@AaronNGray
Copy link

BTW Do seriously look at the generated code from TypeScript its not very nice as compared to the type erased code from Flow.

@SimenB
Copy link
Member Author

SimenB commented Jan 18, 2019

We'll be using babel to transpile, so it'll look the same as our current code does.

@rstuven
Copy link

rstuven commented Jan 19, 2019

Personally, I've been a part of migrating all of Artsy's JavaScript to TypeScript, and while it's a little bit of churn at first (notably the .js -> .ts kills git history, long PRs need a harder rebase, etc) it;'s paid off in terms of dev tooling across the board

@orta If you use git mv then git will preserve history. I'm sorry it's probably too late to rescue your repo's history, but might be useful next time :)

@orta @ciszak Or, if you renamed without using git mv, you can later use git add -A which "adds, modifies, and removes index entries to match the working tree." (https://git-scm.com/docs/git-add#git-add--A)

@lukepighetti

This comment has been minimized.

@AaronNGray

This comment has been minimized.

@jednano

This comment has been minimized.

@lukepighetti

This comment has been minimized.

@SimenB
Copy link
Member Author

SimenB commented Jan 20, 2019

Please don't discuss transpiled output from tsc it here - it is completely off topic (we won't even use tsc to generate the code).

Also please don't discuss ways to keep git history in files - this issue should be focused on the the why's of the migration to TS, not the how's (at least not at that level - gotchas from other experiences migrating code is of course welcome! 🙂).

@dgreene1
Copy link

Thank you so much. Type safety in tests is a huge boost of productivity, especially when creating mock objects.

Speaking of which, here's a shameless plug for a thin wrapper library I wrote that keeps your mocks typesafe.

If it's of any use the core Jest team (@SimenB, et. al.) then I would be happy to fold it into Jest and to deprecate my library. :) Until then, maybe some of you will find it useful. :)

https://github.com/dgreene1/typeSafeJestSpy

@SimenB
Copy link
Member Author

SimenB commented Feb 5, 2019

Jest 24.1.0 is out, so I've opened up a tracking issue for the migration: #7807. Any help and feedback people can provide is very much appreciated 🙂

@SimenB
Copy link
Member Author

SimenB commented Mar 4, 2019

@dgreene1 sorry, I somehow missed your comment! I think it is type safe now, though? We've copied over from DT, which relatively recently landed it (DefinitelyTyped/DefinitelyTyped#32579 and a bunch of follow-ups/linked issues)

You're more than welcome to go through our implementation and see if it can be improved: https://github.com/facebook/jest/blob/5ba35be3ef64090bae5e04f51fd170529fba3d1a/packages/jest-mock/src/index.ts

@SimenB
Copy link
Member Author

SimenB commented Mar 5, 2019

As a follow-up to this - we've successfully completed the migration of all packages. An alpha has been published as [email protected]. This should not be a breaking change, but we wanted to publish it as an alpha first to be safe

@Andarist
Copy link
Contributor

Andarist commented Mar 5, 2019

@SimenB How did you migrate such a big codebase would be an interesting subject for a blog post 😉

@SimenB
Copy link
Member Author

SimenB commented Mar 5, 2019

Yeah, @cpojer said the same... I'll be writing one, no promises about when though

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.