[MM-20522] Migrate rhs_card to TypeScript #5354

AkaZecik · 2020-04-19T23:16:19Z

Summary

Migrates components/rhs_card to TypeScript

Ticket Link

Description

There are 4 commits. First two migrate components/rhs_card to TypeScript. Last two attempt to remove redundancy by marking constants as const. The problem is that if we wanted to get a type which is equal to a union of values from an object, we had to extract them manually:

const RHSStates = {
    MENTION: 'mention',
    SEARCH: 'search',
    FLAG: 'flag',
    PIN: 'pin',
    PLUGIN: 'plugin',
} /* as const */;
type RhsStates = 'mention' | 'search' | 'flag' | 'pin' | 'plugin'; // unnecessary redundancy

By marking RHSStates as const, we can extract values automatically:

type ValueOf<T> = T[keyof T];
type RhsStates = ValueOf<typeof RHSStates> // it's 'mention' | 'search' | 'flag' | 'pin' | 'plugin'

Withouth as const, the type in the last line would be string instead (due to type widening).

First two commits pass npm run check-types and completely solve the mentioned ticket.

However, last two commits don't, because TypeScript can't resolve webpack's inline loaders in utils/constants.tsx (there are four imports, all of form import SomeCSS from '!!file-loader?name=files/code_themes/[hash].[ext]!highlight.js/styles/FILE.css').

I've spent quite some time researching what the reason is and how to solve it, but unfortunately I wasn't able.

If you prefer, you can revert last two commits from this PR and merge it, or perhaps make some changes to webpack's/typescript's configuration so that those 4 imports won't offend TypeScript anymore :)

Many thanks to Abdusabri (for this reason 🥇).

AkaZecik · 2020-04-19T23:23:34Z

components/post_profile_picture/post_profile_picture.jsx

-        enablePostIconOverride: PropTypes.bool.isRequired,
-        hasImageProxy: PropTypes.bool.isRequired,
+        enablePostIconOverride: PropTypes.bool,
+        hasImageProxy: PropTypes.bool,


Direct quote from Abdusabri's PR (link):

The component was already used without providing enablePostIconOverride and hasImageProxy props. Like:

const avatar = ( <PostProfilePicture compactDisplay={false} post={selected} userId={selected.user_id} /> );

So, had to make them optional because TypeScript was complaining about this 🙂

AkaZecik · 2020-04-19T23:24:48Z

components/post_profile_picture/post_profile_picture.jsx

        isBusy: PropTypes.bool,
        isRHS: PropTypes.bool,
        post: PropTypes.object.isRequired,
        status: PropTypes.string,
        user: PropTypes.object,
        isBot: PropTypes.bool,
        postIconOverrideURL: PropTypes.string,
+        userId: PropTypes.string


Direct quote from Abdusabri's PR (link):

Was provided to the component when used in rhs_card, I've added it as optional to please the TypeScript compiler 🙂

It doesn't appear to be used in this component though, so we should remove it and all references to it instead.

AkaZecik · 2020-04-19T23:27:51Z

types/store/plugins.ts

 };

 export type PostPluginComponent = {
    id: string;
    pluginId: string;
    type: string;
-    component: React.Component;
+    component: React.ComponentType<{post: Post}>;


Direct quote from Abdusabri's PR (link):

When a react component is used like in the following code snippet:

if (pluginPostCardTypes.hasOwnProperty(postType)) { const PluginComponent = pluginPostCardTypes[postType].component; content = <PluginComponent post={selected}/>; }

TypeScript is looking for a type not a constructor function, so it was giving an error that the PluginComponent variable does not have any construct or call signatures. So, had to change it.

Here are couple of links for reference:

microsoft/TypeScript#28631

https://stackoverflow.com/questions/31815633/what-does-the-error-jsx-element-type-does-not-have-any-construct-or-call

AkaZecik · 2020-04-19T23:30:10Z

types/store/rhs.ts

@@ -17,5 +19,3 @@ export type RhsViewState = {
    isSidebarExpanded: boolean;
    isMenuOpen: boolean;
 };
-
-export type RhsState = 'mention' | 'search' | 'flag' | 'pin' | 'plugin';


This type was only used from components/rhs_card and from this file. Every occurrence was replaced with ValueOf<typeof RHSStates>

AkaZecik · 2020-04-19T23:32:21Z

utils/constants.tsx

-import solarizedLightCSS from '!!file-loader?name=files/code_themes/[hash].[ext]!highlight.js/styles/solarized-light.css'; // eslint-disable-line import/order
+import solarizedLightCSS from '!!file-loader?name=files/code_themes/[hash].[ext]!highlight.js/styles/solarized-light.css';
+
+export type ValueOf<T> = T[keyof T];


This generic type allows getting union of values from a const object in a single line: ValueOf<typeof my_obj>

I think we're good with this. Thanks :)

AkaZecik · 2020-04-19T23:34:04Z

utils/constants.tsx


 export const RHSStates = {
    MENTION: 'mention',
    SEARCH: 'search',
    FLAG: 'flag',
    PIN: 'pin',
    PLUGIN: 'plugin',
-};
+} as const;


Allows for ValueOf<typeof RHSStates> instead of 'mention' | 'search' | 'flag' | 'pin' | 'plugin'

AkaZecik · 2020-04-19T23:49:07Z

utils/constants.tsx

-        Permissions.MANAGE_CREATE_EMOJIS,
-        Permissions.MANAGE_DELETE_EMOJIS,
+        Permissions.CREATE_EMOJIS,
+        Permissions.DELETE_EMOJIS,


This change is based on the fact that at the time these two lines were added (and now as well), there were no such permissions in mattermost-redux/src/constants/permissions.js and no such permissions were referenced in mattermost-web nor mattermost-redux.

Commits that introduced this change:

mattermost-webapp: Using new splited emojis and webhooks permissions (#2339)

mattermost-redux: Splitting emojis and webhooks permissions (#4532)

Updating mattermost-redux to the latest commit and updating your branch to master should resolve the need for this.

Hmm, I've fetched master for both mattermost-webapp and mattermost-redux and I've run npm update afterwards, but this line still gives me an error TS2551: Property 'MANAGE_CREATE_EMOJIS' does not exist on type (...) and same for MANAGE_DELETE_EMOJIS. I also don't see them in mattermost-redux/src/constants/permissions.ts.

@jespino Should utils/constants.tsx be updated on master here?

Yes, I think so, I think the current state is a bug, and we should change that.

hanzei · 2020-04-20T05:51:12Z

@AkaZecik Thanks for opening this PR! I've queued it for review.

Heads up that there is a merge conflict. Would you please resolve it?

Also the CI failed with:

utils/constants.tsx:33:23 - error TS2307: Cannot find module '!!file-loader?name=files/code_themes/[hash].[ext]!highlight.js/styles/github.css'.

33 import githubCSS from '!!file-loader?name=files/code_themes/[hash].[ext]!highlight.js/styles/github.css';
                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

utils/constants.tsx:36:24 - error TS2307: Cannot find module '!!file-loader?name=files/code_themes/[hash].[ext]!highlight.js/styles/monokai.css'.

36 import monokaiCSS from '!!file-loader?name=files/code_themes/[hash].[ext]!highlight.js/styles/monokai.css';
                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

utils/constants.tsx:39:30 - error TS2307: Cannot find module '!!file-loader?name=files/code_themes/[hash].[ext]!highlight.js/styles/solarized-dark.css'.

39 import solarizedDarkCSS from '!!file-loader?name=files/code_themes/[hash].[ext]!highlight.js/styles/solarized-dark.css';
                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

utils/constants.tsx:42:31 - error TS2307: Cannot find module '!!file-loader?name=files/code_themes/[hash].[ext]!highlight.js/styles/solarized-light.css'.

42 import solarizedLightCSS from '!!file-loader?name=files/code_themes/[hash].[ext]!highlight.js/styles/solarized-light.css';
                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Please let me know if you need help resolving the failure.

AkaZecik · 2020-04-20T08:45:51Z

Merge conflicts are now resolved, I must have pulled from my fork's master. Sorry for that.

utils/constants.tsx:33:23 - error TS2307: Cannot find module '!!file-loader?name=files/code_themes/[hash].[ext]!highlight.js/styles/github.css'.
utils/constants.tsx:36:24 - error TS2307: Cannot find module '!!file-loader?name=files/code_themes/[hash].[ext]!highlight.js/styles/monokai.css'.
utils/constants.tsx:39:30 - error TS2307: Cannot find module '!!file-loader?name=files/code_themes/[hash].[ext]!highlight.js/styles/solarized-dark.css'.
utils/constants.tsx:42:31 - error TS2307: Cannot find module '!!file-loader?name=files/code_themes/[hash].[ext]!highlight.js/styles/solarized-light.css'.

Please let me know if you need help resolving the failure.

Those lines fail, because I changed the extension from .jsx to .tsx and it seems that webpack can't understand those lines anymore. There is a broader comment about it in the description of this PR. I will appreciate any help with that.

devinbinnie

Thanks for the contribution @AkaZecik, I have some comments shown below.

devinbinnie · 2020-04-20T19:37:16Z

components/post_profile_picture/post_profile_picture.jsx

        isBusy: PropTypes.bool,
        isRHS: PropTypes.bool,
        post: PropTypes.object.isRequired,
        status: PropTypes.string,
        user: PropTypes.object,
        isBot: PropTypes.bool,
        postIconOverrideURL: PropTypes.string,
+        userId: PropTypes.string


It doesn't appear to be used in this component though, so we should remove it and all references to it instead.

devinbinnie · 2020-04-20T19:40:21Z

components/rhs_card/rhs_card.tsx

+    pluginPostCardTypes: PluginsState['postCardTypes'];
+    previousRhsState?: ValueOf<typeof RHSStates>;
+    enablePostUsernameOverride?: boolean;
+    teamUrl: string;


Should be optional according to the original spec.

devinbinnie · 2020-04-20T19:40:32Z

components/rhs_card/rhs_card.tsx

+
+interface Props {
+    selected?: Post;
+    pluginPostCardTypes: PluginsState['postCardTypes'];


Should be optional according to the original spec.

devinbinnie · 2020-04-20T19:40:49Z

components/rhs_card/rhs_card.tsx

+    previousRhsState?: ValueOf<typeof RHSStates>;
+    enablePostUsernameOverride?: boolean;
+    teamUrl: string;
+    channel?: Channel;


This isn't used in the component anywhere and should be removed.

It was present in tests, so I decided to include it here. I will remove it from there as well

devinbinnie · 2020-04-20T19:41:16Z

components/rhs_card/rhs_card.tsx

-        teamUrl: PropTypes.string,
-    }
+export default class RhsCard extends React.Component<Props, State> {
+    state: State;


state is already a property of React.Component, you don't need to declare it.

I added it based on this suggestion from react-typescript-cheatsheet (there is a subsection called 'Why annotate state twice?', which references awesome advice from Ferdaber #57)

I can remove it, if you prefer

We usually do it in the constructor if it is required. If we aren't using state at all then we just don't declare the type. I haven't run into any issues using this.setState with TypeScript so I think we're okay to remove it.

devinbinnie · 2020-04-20T19:43:51Z

types/store/plugins.ts

 };

 export type AdminConsolePluginComponent = {
    pluginId: string;
    key: string;
-    component: React.Component;
+    component: React.ComponentType<any>;


Is this really any component or is it a certain subset of components? We try to avoid any so it'd be nice to type this.

I tried to go through it's usages, but I couldn't find anything that would suggest it's shape. I could replace any with {} which would postpone this problem to whenever some other migration from JS to TS that uses it would start complaining. Running npm run check-types with {} instead of any reports no new errors. What are your thoughts?

Both are kinda the same thing, since they basically just short-circuit type checking by allowing basically anything. Usually the type checker should throw an error if you type it wrong, so maybe that could suggest it's shape? I'm 2/5 on this, if it's too much effort I'm okay with a comment explaining why the any or {} was used.

Wouldn't React.ComponentType<{}> allow only for components without any properties? For example, when I remove {post: Post} from above, I get an error here: components/rhs_card/rhs_card.tsx:120.

With {}, instantiating a component with any props would raise an error and would require an update to the shape here.

I will try to dig deeper to find a shape, I will let you know how it goes

devinbinnie · 2020-04-20T19:45:01Z

utils/constants.tsx

-        Permissions.MANAGE_CREATE_EMOJIS,
-        Permissions.MANAGE_DELETE_EMOJIS,
+        Permissions.CREATE_EMOJIS,
+        Permissions.DELETE_EMOJIS,


Updating mattermost-redux to the latest commit and updating your branch to master should resolve the need for this.

devinbinnie · 2020-04-20T19:45:36Z

utils/constants.tsx

-import solarizedLightCSS from '!!file-loader?name=files/code_themes/[hash].[ext]!highlight.js/styles/solarized-light.css'; // eslint-disable-line import/order
+import solarizedLightCSS from '!!file-loader?name=files/code_themes/[hash].[ext]!highlight.js/styles/solarized-light.css';
+
+export type ValueOf<T> = T[keyof T];


I think we're good with this. Thanks :)

- remove userId from PostProfilePicture and all references to it - remove channel from RhsCard and it's tests - make teamUrl optional in RhsCard - remove explicit declaration of state in RhsCard - change shape of component from any to {} (temporary)

calebroseland

🎉LGTM

mattermod · 2020-08-01T01:02:24Z

This issue has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @hanzei

jasonblais · 2020-08-02T23:57:24Z

/update-branch

mattermod · 2020-08-02T23:57:26Z

Error trying to update the PR.
Please do it manually.

jasonblais · 2020-08-02T23:57:48Z

@devinbinnie I believe this is pending your review next. Are you open to helping with that?

devinbinnie · 2020-08-04T13:22:29Z

Hey @AkaZecik, looks like we have some type checking errors on this PR. Can you resolve those and update the branch to master before we proceed? Thanks :)

AkaZecik · 2020-08-16T08:41:23Z

Hi, sorry for the delays, I'm quite busy recently. If anyone wants to pick this up, feel free to go. Otherwise, when I find a spare time and it stays in the same place, I will fix it, but I can't give the time yet.

From what I remember, there was an issue with webpack in file utils/constants.tsx that I was stuck on. I commented about it above. If someone could help me with just this part, that would move this PR a lot further. I modified this file to add a handy generic type, but we can live without it, so if the problem with webpack is big, we might want to revert changes in this file.

devinbinnie · 2020-08-17T13:54:51Z

Hi, sorry for the delays, I'm quite busy recently. If anyone wants to pick this up, feel free to go. Otherwise, when I find a spare time and it stays in the same place, I will fix it, but I can't give the time yet.

From what I remember, there was an issue with webpack in file utils/constants.tsx that I was stuck on. I commented about it above. If someone could help me with just this part, that would move this PR a lot further. I modified this file to add a handy generic type, but we can live without it, so if the problem with webpack is big, we might want to revert changes in this file.

@AkaZecik What I think we can do here for now is to add our own module for webpack in types/external and add this line to it:
declare module '!!file-loader*';

That should take care of those particular TS errors.

mattermod · 2020-08-28T01:01:21Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich

jfrerich · 2020-08-28T14:31:22Z

@devinbinnie, @calebroseland, @jespino it sounds like the author is quite busy. Any chance one of you could pick up the PR and push it through?

jespino · 2020-08-28T14:35:01Z

@jfrerich I don't mind to give it a try, but i prefer to have the explicit consent from @AkaZecik, I think there is no rush in having it migrated and if he wants to cross the line by himself, I think it is ok to wait him. If he is ok about make the final steps from our side, I think is ok too.

jfrerich · 2020-08-28T14:39:16Z

Thanks, @jespino and I agree we should wait for approval from @AkaZecik :) If you don't feel there is a rush to complete this PR, that's fine too. I recently took ownership of addressing the stale tickets (from @hanzei) and will be chiming in once a ticket goes stale. I'm won't be pushy, but just chime in on the stale ticket notifications :)

jespino · 2020-08-28T14:41:29Z

@jfrerich yes, sounds great, we can give @AkaZecik a grace period of 15 days or so, and if he doesn't answer, we can move forward, but I prefer to give him the opportunity of share his preference.

AkaZecik · 2020-08-29T17:35:47Z

Hi, I'm OK with one of you finishing this PR 😃 I can't tell when I will have time to do it myself and I would rather not stall any progress here

hanzei · 2020-09-22T10:20:35Z

@jespino @devinbinnie @calebroseland Is one of you picking up this PR?

devinbinnie · 2020-09-22T13:21:11Z

@jespino @devinbinnie @calebroseland Is one of you picking up this PR?

@hanzei Not sure if any of us have time to do so, would it be alright to open the ticket to Up for Grabs again with this PR as a starting point for any contributor to finish it off?

hanzei · 2020-09-22T13:33:08Z

@AkaZecik Would you be fine closing your PR and using the as a starting point for another community member?

AkaZecik · 2020-09-23T01:34:48Z

It's ok :) I will reopen it if I come back to this issue before anyone else.

Artur Zubilewicz added 4 commits April 19, 2020 23:12

Rename files

ca65858

Migrate components/rhs_card to TypeScript

6692707

Change extension of utils/constants.jsx to .tsx

8e87093

Apply 'as const' assertion to remove redundancy

05da7fd

AkaZecik commented Apr 19, 2020

View reviewed changes

hanzei requested review from sudheerDev and devinbinnie April 20, 2020 05:48

hanzei added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Apr 20, 2020

Artur Zubilewicz added 2 commits April 20, 2020 10:12

Merge branch 'master' into MM-20522_migrate-rhs_card-to-typescript

a0026c4

Fix import of utils/constants.tsx

ffa369c

devinbinnie suggested changes Apr 20, 2020

View reviewed changes

Artur Zubilewicz added 2 commits April 21, 2020 16:24

Apply suggestions from review

3455aac

- remove userId from PostProfilePicture and all references to it - remove channel from RhsCard and it's tests - make teamUrl optional in RhsCard - remove explicit declaration of state in RhsCard - change shape of component from any to {} (temporary)

Merge branch 'master' into MM-20522_migrate-rhs_card-to-typescript

a9c3fa0

AkaZecik changed the title ~~Mm 20522 migrate rhs card to typescript~~ [MM-20522] Migrate rhs_card to TypeScript Apr 21, 2020

Update snapshots

70a61ff

sudheerDev requested review from calebroseland and removed request for sudheerDev April 30, 2020 14:37

calebroseland approved these changes May 7, 2020

View reviewed changes

mattermod added the Lifecycle/1:stale label Aug 1, 2020

devinbinnie removed the Lifecycle/1:stale label Aug 4, 2020

hanzei added the Awaiting Submitter Action Blocked on the author label Aug 7, 2020

mattermod added the Lifecycle/1:stale label Aug 28, 2020

hanzei removed 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Sep 22, 2020

hanzei added Llifecycle/3:orphaned and removed Awaiting Submitter Action Blocked on the author Lifecycle/1:stale labels Sep 23, 2020

hanzei closed this Sep 23, 2020

[MM-20522] Migrate rhs_card to TypeScript #5354

[MM-20522] Migrate rhs_card to TypeScript #5354

Conversation

AkaZecik commented Apr 19, 2020 • edited

Summary

Ticket Link

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkaZecik Apr 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkaZecik Apr 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanzei commented Apr 20, 2020

AkaZecik commented Apr 20, 2020 • edited

devinbinnie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkaZecik Apr 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkaZecik Apr 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebroseland left a comment

Choose a reason for hiding this comment

mattermod commented Aug 1, 2020

jasonblais commented Aug 2, 2020

mattermod commented Aug 2, 2020

jasonblais commented Aug 2, 2020

devinbinnie commented Aug 4, 2020

AkaZecik commented Aug 16, 2020

devinbinnie commented Aug 17, 2020

mattermod commented Aug 28, 2020

jfrerich commented Aug 28, 2020

jespino commented Aug 28, 2020

jfrerich commented Aug 28, 2020

jespino commented Aug 28, 2020

AkaZecik commented Aug 29, 2020

hanzei commented Sep 22, 2020

devinbinnie commented Sep 22, 2020

hanzei commented Sep 22, 2020

AkaZecik commented Sep 23, 2020

AkaZecik commented Apr 19, 2020 •

edited

AkaZecik Apr 19, 2020 •

edited

AkaZecik Apr 19, 2020 •

edited

AkaZecik commented Apr 20, 2020 •

edited

AkaZecik Apr 21, 2020 •

edited

AkaZecik Apr 21, 2020 •

edited