Move to Typescript #36

denieler · 2019-01-24T17:32:11Z

Description

This PR is about moving Picasso to Typescript. I want you guys to review the changes has been made and discuss them. The most important once I would like to talk:

Plan of the components migration to *.tsx
eslint doesn't really make sense for TS, I faced already few problems with it: PropTypes validation, matching Default prop types to PropTypes and, more important, TS types imports from *.d.ts which is not supported by eslint. So as a proposition - having tslint (https://github.com/palantir/tslint) and keep using eslint until we move 100% to TS

bytasv · 2019-01-25T08:34:54Z

Talking about linting - are there alternatives for standard/prettier, react and other eslint presets in tslint?
Also can't eslint work with TS code?

denieler · 2019-01-25T09:55:27Z

@bytasv it looks like there are react-related tslint rules: https://github.com/palantir/tslint-react
but as for me they look worse, than what we have now for eslint react.

For eslint there is existing a parser for TS code https://github.com/eslint/typescript-eslint-parser , but after I tried it, it looks like, it was complaining about imported type from MaterialUI. Something like no used import, but I will double-check.

bytasv · 2019-01-25T10:18:53Z

Ok, so apparently the obvious way is to stick with ESlint: bradzacher/eslint-plugin-typescript#290

Here is official statement from TS team: microsoft/TypeScript#29288 so we should just take that into consideration, drop TS lint for now I guess and see what can we do to use ESlint

denieler · 2019-01-25T10:26:51Z

@bytasv yeah, this microsoft/TypeScript#29288 makes a lot of sense 👍 thanks for finding this!

components/Label/Label.tsx

components/Label/styles.ts

components/Label/test.tsx

components/Label/Label.tsx

MilosMosovsky · 2019-01-28T14:20:51Z

components/Label/Label.tsx

+  classes: {},
+  label: undefined,
+  onDelete: undefined,
+  variant: undefined


This probably shouldn't be undefined and it should have some default variant

we can use default for example, but this variant is our variant which controls the root class which we set for Chip component. But I think it just adds complexity if we specify default instead of just checking for undefined:

const rootClass = variant ? classes[variant] : ''

wdyt @MilosMosovsky ?

components/Label/Label.tsx

components/Label/styles.ts

components/Label/Label.tsx

package.json

components/TextField/TextField.jsx

denieler · 2019-01-29T08:41:57Z

package.json

  "dependencies": {
-    "@material-ui/core": "^3.6.0",
+    "@material-ui/core": "3.6.0",


important to fix this version, because snapshot tests are failing with updating MUI core. Maybe we can use Greenkeeper or something like that to keep us updated?

denieler · 2019-01-29T08:42:42Z

package.json

@@ -68,7 +80,7 @@
    "eslint-plugin-import": "^2.14.0",
    "eslint-plugin-node": "^8.0.0",
    "eslint-plugin-promise": "^4.0.1",
-    "eslint-plugin-react": "^7.11.1",
+    "eslint-plugin-react": "7.11.1",


Important to fix it, with the new version a lot of new linting problems are appearing. We should update it, but in the other PR

package.json

tsconfig.json

bin/build

Add config, packages and ts linter Fix import of React and PropTypes Fix color import Remove noEmit config from ts config Fix unit tests with correct import of color Support ES6 modules and babel imports in TS This information was really helpful - https://blogs.msdn.microsoft.com/typescript/2018/01/31/announcing-typescript-2-7/#easier-ecmascript-module-interoperability Move Label component to tsx Fix usage of eslint for Typescript Enable linting for ts files Move styles, test files to TS Update component generator to use TS Fix default imports for react and prop-types packages Remove tslint config Cleanup tsconfig Fix the issue with linting of imported types for typescript Remove some classes type casting for Label Remove webpack and replace with tsc cli Add 2 configurations for tsc: ES6 and ES2015 Return ts-loader for storybook Fix version of material-ui core Fix linting Simplify classnames for Label Move default props to the bottom of the component Fix build script with using Typescript to compile and remove all babel dependencies Add skiplibcheck to tsconfig Leave only 1 tsconfig, build only ES6/ES2015 version

bytasv · 2019-01-29T11:38:07Z

components/Label/Label.example.jsx

@@ -39,14 +39,14 @@ chapter.addSection('Flat', null, () => (
 ))

 chapter.addSection('Statuses', 'Use these to communicate status.', () => (
-  <React.Fragment>
+  <>


Do all IDEs support it properly? Or at least the ones that we use?

I use Webstorm

cc @MilosMosovsky

bytasv

Looking forward ✅

denieler added the DO NOT MERGE label Jan 24, 2019

denieler requested a review from a team January 24, 2019 17:32

denieler force-pushed the picas-39-move-to-typescript branch from 1d3dcb5 to 6833c07 Compare January 25, 2019 12:18

bytasv force-pushed the master branch from 38c72bf to f544ea8 Compare January 25, 2019 13:21

denieler force-pushed the picas-39-move-to-typescript branch from 8efe9bf to 423d9b5 Compare January 25, 2019 13:47

denieler commented Jan 25, 2019

View reviewed changes

components/Label/Label.tsx Show resolved Hide resolved

denieler commented Jan 25, 2019

View reviewed changes

components/Label/styles.ts Show resolved Hide resolved

denieler commented Jan 25, 2019

View reviewed changes

components/Label/test.tsx Outdated Show resolved Hide resolved

MilosMosovsky reviewed Jan 28, 2019

View reviewed changes

bytasv reviewed Jan 28, 2019

View reviewed changes

components/Label/Label.tsx Outdated Show resolved Hide resolved

bytasv reviewed Jan 29, 2019

View reviewed changes

package.json Show resolved Hide resolved

denieler commented Jan 29, 2019

View reviewed changes

components/TextField/TextField.jsx Show resolved Hide resolved

denieler commented Jan 29, 2019

View reviewed changes

denieler removed the DO NOT MERGE label Jan 29, 2019

MilosMosovsky approved these changes Jan 29, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

tsconfig.json Show resolved Hide resolved

bin/build Outdated Show resolved Hide resolved

denieler force-pushed the picas-39-move-to-typescript branch from 6f9bbad to d25c109 Compare January 29, 2019 11:23

denieler force-pushed the picas-39-move-to-typescript branch from 35d89af to 310dd45 Compare January 29, 2019 11:30

bytasv reviewed Jan 29, 2019

View reviewed changes

bytasv approved these changes Jan 29, 2019

View reviewed changes

denieler merged commit ea6bbc8 into master Jan 29, 2019

denieler deleted the picas-39-move-to-typescript branch January 29, 2019 11:46

MilosMosovsky added the Chore Everything not related to a new features label Jan 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to Typescript #36

Move to Typescript #36

denieler commented Jan 24, 2019

bytasv commented Jan 25, 2019

denieler commented Jan 25, 2019

bytasv commented Jan 25, 2019

denieler commented Jan 25, 2019

MilosMosovsky Jan 28, 2019

denieler Jan 29, 2019 •

edited

Loading

denieler Jan 29, 2019

denieler Jan 29, 2019

bytasv Jan 29, 2019

bytasv left a comment

Move to Typescript #36

Move to Typescript #36

Conversation

denieler commented Jan 24, 2019

Description

bytasv commented Jan 25, 2019

denieler commented Jan 25, 2019

bytasv commented Jan 25, 2019

denieler commented Jan 25, 2019

MilosMosovsky Jan 28, 2019

Choose a reason for hiding this comment

denieler Jan 29, 2019 • edited Loading

Choose a reason for hiding this comment

denieler Jan 29, 2019

Choose a reason for hiding this comment

denieler Jan 29, 2019

Choose a reason for hiding this comment

bytasv Jan 29, 2019

Choose a reason for hiding this comment

bytasv left a comment

Choose a reason for hiding this comment

denieler Jan 29, 2019 •

edited

Loading