[POC] Incremental switch to Typescript by rosskevin · Pull Request #9517 · mui/material-ui

rosskevin · 2017-12-16T14:27:43Z

The goal

The goal of this PR is to as minimally as possible get to the typescript tool chain to allow us to incrementally convert to typescript.

This POC (as-is) proves

Typescript can be used to interpret ts, tsx, and js files
Typescript can run a TS spec with js source, specifically Paper.spec.tsx

Sticky transition points

modules with module.exports = warning are different in TS, they need to use import deepmerge = require('deepmerge') and this workaround can only be applied in a .ts file. In an effort to minimize this POC and keep the filesjs for the moment - I created a folder /utils/ts-import-workaround with relevant re-exports. As soon as the files are moved from js to ts, this is totally unnecessary
eslint for the above issue, and eslint in general. We can momentarily use eslint-disable-line on the above imports so that eslint is happy with the js. As (or if) we move to TS, we should consider moving to tslint instead of eslint. Smarter type aware linting with much of the same rules are available, though not as extensive as eslint. There are more options here

Advantages

In the minimized amount of work during this process I uncovered several TS definition bugs in our code. These were trivial but nonetheless - doing something like this will firm up our use of TS definitions and/or ts files themselves. And, of course, this also gives us a way to move to fully typed TS at our own pace.

Next steps

If we decide to move ahead, move through the rest of the typescript errors in the project which are:

remove flow from remaining files - it is not valid js and therefore not valid ts
fix imports
add ts-loader to webpack (ts-loader first then babel-loader)

When removing flow on utility files, it might be just as easy to swap them to ts at that point and convert the flow types.

oliviertassinari · 2017-12-16T14:40:41Z

+546 −29,580

@rosskevin Maybe we should removeflow-typed folder in a different PR? GitHub makes it very hard to see what changed.

rosskevin · 2017-12-16T14:58:55Z

I put back those files to prevent the diff. You can ignore changes in the .d.ts files - prettier was not running on those previously and now is.

sebald · 2017-12-16T15:00:36Z

Wow, looks like this is really happening?! 🙃 Thanks @rosskevin for starting the effort.

As @oliviertassinari already mentioned removing the flow-types in another PR would be nice. It's a bit scary to review a PR with 178 changed files.

I'll will have more time on Monday to take a look. Regarding your "sticky transition points":

Isn't import * as warning from 'warning'* working for you? See: https://codesandbox.io/s/yvym9w2wj
Personally, not the biggest fan of tslint, but we use it too at my company, since it is much easier to setup than eslint + typescript. If we want to use eslint while converting components to TypeScript, we could try out this: https://github.com/eslint/typescript-eslint-parser

rosskevin · 2017-12-16T15:03:54Z

@sebald the import * as warning from 'warning'; is working now. Not sure at what stage I had that problem so I'll yank it. I already put back the flow-typed files

rosskevin · 2017-12-16T15:08:30Z

@sebald these are the errors that require the import workaround:

TypeError: deepmerge_1.default is not a function

I tried going back on deep merge to the import as * with no luck.

rosskevin · 2017-12-16T16:40:49Z

I'm continuing some conversion to ts of internal code on a branch - trying to inch forward to something mergeable. When all of you are done with review, I can merge my other branch to this PR.

oliviertassinari · 2017-12-16T16:41:40Z

-import classNames from 'classnames';
-import warning from 'warning';
+import * as React from 'react';
+import * as PropTypes from 'prop-types';


It seems required for React: https://github.com/react-toolbox/react-toolbox/blob/agnostic-typescript/packages/react-toolbox-core/src/components/Dialog/Dialog.tsx#L1 But why do we need this change for the other imports?

Most imports from non-ES2015+ modules are effectively import namespace as, which means the module's exports plus types (if any) - import them as a single named variable. So module.exports = warning is both not a named export AND not a default export, which requires this syntax to effectively get all exports as warning.

Notice that our own imports with default exports need none of this, so as we eventually graduate to ES2015 modules we won't need these even with external modules, unless we choose to import all their types as well with that variable.

Effectively - this is valid spec javascript.

Thanks for the explanation.

Well, we have the allowSyntheticDefaultImports flag, that can make things like "import React from 'react'" valid

see: https://www.typescriptlang.org/docs/handbook/compiler-options.html
and: https://javascriptplayground.com/blog/2017/04/react-typescript/

I think that we need to be conservative and initially just change only the syntax that are really invalid

The allowSyntheticDefaultImports flag doesn't actually change the semantics of the generated code, it just disables the error AFAIK, so this is not a good thing for libraries to do IMO. Webpack may compensate for it, but your tests running in node would fail. There's a feature coming in TypeScript 2.7 which will bring the semantics in line with Babel's however.

That has also been my experience @pelotom. Turning it on just helps find the errors quicker.

rosskevin · 2017-12-16T18:11:15Z

I've done the src/styles conversion - src files only. I skipped withSyles|withTheme for the moment. Need to eat.
https://github.com/alienfast/material-ui/tree/ts-switch-continued

oliviertassinari · 2017-12-16T18:47:17Z

Question, does it make sense to try to convert one component here too?

rosskevin · 2017-12-16T19:12:21Z

I can, but no it won't matter. I'm going to work on my continued branch to see if I can solve all TS errors and make tests run.

pelotom · 2017-12-18T01:24:23Z

    "jsx": "react",
-    "moduleResolution": "node",
+    "lib": [
+      "es2017",


Which es2017 features are required by material-ui out of curiosity? That seems like a strong requirement to impose on downstream users.

I'm not sure, we can reel that back in when tests run.

rosskevin · 2017-12-18T01:47:48Z

I've got all TS errors fixed, working on tests.
https://github.com/alienfast/material-ui/tree/ts-switch-continued

If everyone is good with this, I'll merge that work here. Just be aware that the diff is mostly useless. I can also just leave this PR as-is, close it and open a new one.

rosskevin · 2017-12-18T05:25:02Z

PR #9535 is up, and it's very close to mergeable.

rosskevin added 10 commits December 15, 2017 21:33

fix createShallow typing

4b66897

convert Paper.spec to tsx

27af50c

attempt to run Paper.spec.tsx

979f7d4

pull flow and omit consoleError for now

eaff0cd

fix failure on <

a96be85

almost running Paper.spec

7908ca9

goodbye flow

b670d6f

broken at Paper.tsx

2b4858b

woohoo Paper.spec.tsx works

60ace94

prettier on ts files

34e0471

rosskevin requested review from nathanmarks, oliviertassinari, pelotom and sebald December 16, 2017 14:27

rosskevin added 2 commits December 16, 2017 08:56

add back flow, flow-typed to prevent diff

8755b11

prevent flowconfig diff

595e601

fix import * as warning from 'warning';

21ca9a9

oliviertassinari added the typescript label Dec 16, 2017

rosskevin added 4 commits December 16, 2017 09:35

deepmerge workaround

fe5eacb

add back consoleError in test setup

e065f58

fix export MuiShallow

85f3277

fix bad replacements of : Object in .d.ts files

e280a41

oliviertassinari reviewed Dec 16, 2017

View reviewed changes

pelotom reviewed Dec 18, 2017

View reviewed changes

rosskevin mentioned this pull request Dec 18, 2017

Switch to Typescript incrementally (test runs ts-node) #9535

Closed

rosskevin closed this Dec 18, 2017

Uh oh!

Conversation

rosskevin commented Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The goal

This POC (as-is) proves

Sticky transition points

Advantages

Next steps

Uh oh!

oliviertassinari commented Dec 16, 2017

Uh oh!

rosskevin commented Dec 16, 2017

Uh oh!

sebald commented Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rosskevin commented Dec 16, 2017

Uh oh!

rosskevin commented Dec 16, 2017

Uh oh!

rosskevin commented Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rosskevin commented Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliviertassinari commented Dec 16, 2017

Uh oh!

rosskevin commented Dec 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rosskevin commented Dec 18, 2017

Uh oh!

rosskevin commented Dec 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rosskevin commented Dec 16, 2017 •

edited

Loading

sebald commented Dec 16, 2017 •

edited

Loading

rosskevin commented Dec 16, 2017 •

edited

Loading

rosskevin commented Dec 16, 2017 •

edited

Loading