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

TASK: rewrite react-ui-components in TypeScript #2128

Merged
merged 19 commits into from
Sep 26, 2018

Conversation

JamesAlias
Copy link
Contributor

@JamesAlias JamesAlias commented Sep 10, 2018

What I did
I started to rewrite the react-ui-components with TypeScript. This is done in preparation for the Neos Sprint in November 2018 where we are going to rewrite the containers and redux store in TypeScript. (That's an assumption not a fact, so correct me if I'm wrong)
It also helps me to get in touch with the code base prior to the Sprint.

I'm not sure whether a single PR or multiple PRs (one for each component) is the best way to go, so just I start with a single PR and we can discuss this later.

There is a lot of discussion needed! (See further below)

Some things I did:

  • Enable TypeScript for Storybook
  • Setup (very strict) tslint rules (open for discussion of course)
  • (WIP) Rewrite react-ui-components in TypeScript

How I did it
I did it with VSCode 🚀 an I suggest you do the same! 😆

How to verify it

  • make storybook
  • make build-watch

TODO

  • upgrade dependencies (most wanted: webpack to v4.x)
  • make lint should also run tslint
  • fix storybook styles (seem broken and some API's are used flat out wrong (e.g. in Icon component)
  • fix tests. hints:
    • tests (make test) doesn't seem to work
    • obviously false code is not catched
    • tests say No tests found
yarn run v1.9.4
$ yarn jest -- -w 2 --coverage
$ NODE_ENV=test jest -w 2 --coverage
No tests found
  184 files checked.
  roots: [...]/neos/Packages/Application/Neos.Neos.Ui/packages/react-ui-components - 184 matches
  testMatch: ./src/**/?(*.)spec.js - 0 matches
  testPathIgnorePatterns: /node_modules/ - 184 matches

I'm exited and looking forward to work with you on this slightly major step in the right direction 😃.

@mstruebing
Copy link
Contributor

Wow, great work!
Thanks for this impressive PR.

I currently don't have time atm to look at it a little bit in depth but hope @dimaip can.
Elsewise I will be able to look at it next week I think.

@JamesAlias JamesAlias force-pushed the typescript-react-ui-components branch from eb5f029 to c313e61 Compare September 11, 2018 13:32
Copy link
Contributor

@dimaip dimaip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, James, I hope you know you are amazing!
Looks good to me by reading, besides two comments.

What I think our plan should be regarding TS support: gradually convert components and make sure the UI keeps on working in between. E.g. I think this PR could be merged once we have tested it extensively.
The bigger change would be converting the redux state, as it would probably involve dropping immutablejs, so that would have to be done in one big PR, but other changes than that could be done gradually, in smaller PRs.

Configuration/Settings.yaml Outdated Show resolved Hide resolved
packages/react-ui-components/.storybook/webpack.config.js Outdated Show resolved Hide resolved
packages/react-ui-components/src/Badge/badge.tsx Outdated Show resolved Hide resolved
@dimaip
Copy link
Contributor

dimaip commented Sep 11, 2018

Btw, see the discussion regarding interface naming: microsoft/TypeScript-Handbook#121

@JamesAlias JamesAlias force-pushed the typescript-react-ui-components branch from c313e61 to ad29d4d Compare September 17, 2018 13:20
@JamesAlias
Copy link
Contributor Author

JamesAlias commented Sep 17, 2018

FYI: I will split this PR up into smaller ones. This way testing and understanding the changes is easier and faster. I'll try to get most of it done by tonight.

@dimaip
Copy link
Contributor

dimaip commented Sep 17, 2018

I'm not sure if this extra work is necessary in this case. If we can give this PR some extra testing, I think it makes sense to merge it, as it looks rather straightforward. But do as you see fit.

Copy link
Contributor Author

@JamesAlias JamesAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These things should be discussed.
@mstruebing @dimaip

How can we test this apart from jest and storybook?

@JamesAlias JamesAlias changed the title [WIP] TASK: rewrite react-ui-components in TypeScript TASK: rewrite react-ui-components in TypeScript Sep 18, 2018
@JamesAlias
Copy link
Contributor Author

JamesAlias commented Sep 19, 2018

@mstruebing @dimaip
Seems like the CI has had a hiccup.

I feel like this PR is ready.
There are some things to discuss but it's not necessary to do it now.

Can you test this or tell me how to test this PR?

How can I rerun the CI? done

@markusguenther
Copy link
Member

markusguenther commented Sep 19, 2018 via email

@JamesAlias JamesAlias force-pushed the typescript-react-ui-components branch from 1b97d5c to ac94ccb Compare September 19, 2018 15:44
Copy link
Member

@skurfuerst skurfuerst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really impressive change! I did not test it in practice, but really looks awesome 👍 👍 👍

Some questions just to ensure it did not get lost:

  • does React Storybook still work for the components?
  • do the Jest Test Cases still work for the components?

tslint.json Show resolved Hide resolved
tslint.json Show resolved Hide resolved
@JamesAlias
Copy link
Contributor Author

JamesAlias commented Sep 25, 2018

@skurfuerst Storybook stories and Jest tests work 😉
Icon styling seems to be broken in Storybook though because the Api of FontAwesome is not used correctly, it seems.

I updated two Jest snapshots, but only for the better 🙂

@skurfuerst
Copy link
Member

perfect, thanks :)

@mstruebing
Copy link
Contributor

So, who wants to press the button ?:D

@dimaip
Copy link
Contributor

dimaip commented Sep 26, 2018

I could try to test the change, but not earlier then the end of the week (project going live...). Feel free to merge without my review, looks good by reading.

Copy link
Contributor

@dimaip dimaip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By reading

@mstruebing mstruebing merged commit 53fa53a into neos:master Sep 26, 2018
@JamesAlias JamesAlias deleted the typescript-react-ui-components branch September 26, 2018 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants