[MM-18953] Migrate 'components/change_url_modal' to TypeScript #5105

haydenhw · 2020-03-23T06:14:09Z

Summary

Migrates 'components/change_url_modal' module and associated tests to TypeScript

Ticket Link

haydenhw · 2020-03-23T17:54:33Z

components/change_url_modal/change_url_modal.test.tsx


- wrapper.instance().onSubmit({preventDefault: jest.fn()});
+ const instance = wrapper.instance() as ChangeURLModal;


The trickiest part of this PR for me was fixing the TS error that appeared when calling methods like onSubmit() on wrapper.instance():
TS2339: Property 'onSubmit' does not exist on type 'Component<{}, {}, any>'.

I found the solution to explicitly declare the type of the instance() method on this SO post.

Curious to see what reviewers think of this.

Fine by me, especially since it's a test. You know what you're casting after all.

haydenhw · 2020-03-23T18:05:52Z

components/change_url_modal/change_url_modal.tsx

 e.preventDefault();
- const url = this.refs.urlinput.value;
+ const url = this.urlInput.current ? this.urlInput.current.value : '';


When I left this simply as const url = this.urlInput.current.value; Typescript gave me this error:
TS2531: Object is possibly 'null'

Not sure if there's a cleaner fix for this than the ternary expression I used?

That seems fine to me.

Yep same for me! I don't see a problem with this.

Actually I just wanted to make an update. Currently our ESlinter doesn't allow this (not setup correctly for Typescript) however in such scenarios, Typescript best practices is to use the optional conditioning (https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#optional-chaining). So in this case it would look something like
this.urlInput?.current?.value.

UPDATE! We have updated ESlint. You can now do
const url = this.urlInput?.current?.value and you should not see the Object is possibly null issue. Let me know how it goes!

Optional conditioning... very cool! It worked out well.

Thanks for the docs link.

cpoile · 2020-03-25T14:47:40Z

@devinbinnie I don't think I'm a good fit for this PR -- I've only done a little TS, so I'm still on the learning curve. Can you switch someone else in please?

devinbinnie · 2020-03-25T14:48:38Z

@cpoile It's important that all staff members get a handle on TypeScript, since all projects are being converted over to use it. Please review, and if you have questions/need any advice, please let me know and I'll be happy to assist :)

cpoile · 2020-03-25T14:53:05Z

@devinbinnie Makes sense. 👍

cpoile

Just a minor question. Otherwise nice work @haydenhw, thank you!

cpoile · 2020-03-25T14:50:48Z

components/change_url_modal/change_url_modal.test.tsx


- wrapper.instance().onSubmit({preventDefault: jest.fn()});
+ const instance = wrapper.instance() as ChangeURLModal;


Fine by me, especially since it's a test. You know what you're casting after all.

cpoile · 2020-03-25T14:52:20Z

components/change_url_modal/change_url_modal.tsx

+ /**
+ * Server error from failed channel creation
+ */
+ serverError: JSX.Element | string | null;


Will this work if it's a JSX.Node? That should allow null assignment (not sure about string though).

The equivalent for PropTypes.node in typescript is actually React.ReactNode. Could we change this to the following:

Suggested change

serverError: JSX.Element | string | null;

serverError?: React.ReactNode;

cpoile · 2020-03-25T14:55:37Z

components/change_url_modal/change_url_modal.tsx

 super(props);
+ this.urlInput = React.createRef();


cpoile · 2020-03-25T14:57:28Z

components/change_url_modal/change_url_modal.tsx

 e.preventDefault();
- const url = this.refs.urlinput.value;
+ const url = this.urlInput.current ? this.urlInput.current.value : '';


That seems fine to me.

hahmadia

Great work! I really like the new types introduced in the function parameters. Helps to keep things tidy!
Just have a few minor suggestions. Otherwise, great work 👍

components/change_url_modal/change_url_modal.tsx

hahmadia · 2020-03-25T18:10:09Z

components/change_url_modal/change_url_modal.tsx

+ /**
+ * Server error from failed channel creation
+ */
+ serverError: JSX.Element | string | null;


The equivalent for PropTypes.node in typescript is actually React.ReactNode. Could we change this to the following:

Suggested change

serverError: JSX.Element | string | null;

serverError?: React.ReactNode;

hahmadia · 2020-03-25T18:36:31Z

components/change_url_modal/change_url_modal.tsx

 e.preventDefault();
- const url = this.refs.urlinput.value;
+ const url = this.urlInput.current ? this.urlInput.current.value : '';


Yep same for me! I don't see a problem with this.

hahmadia · 2020-03-25T18:39:25Z

/update-branch

* Add TS conditionals to optional props * Change this.refs.urlInput.value --> this.urlInput?.current?.value * Change JSX.Element --> React.ReactNode * Remove Props type export

… MM-18953

haydenhw · 2020-03-27T06:24:47Z

components/change_url_modal/change_url_modal.tsx

@@ -146,7 +146,7 @@ export default class ChangeURLModal extends React.PureComponent<Props, State> {

 onSubmit = (e: React.MouseEvent<HTMLButtonElement>) => {
 e.preventDefault();
- const url = this.urlInput.current ? this.urlInput.current.value : '';
+ const url = this.urlInput?.current?.value || '';


Still using a conditional here to make sure url is always a string since the cleanUpUrlable function isn't set up to take null values

hahmadia

Awesome! Thanks for the quick changes. LGTM 👍

hahmadia · 2020-04-01T12:50:59Z

/update-branch

mattermod · 2020-04-01T13:04:42Z

Mattermost test server updated with git commit 597f9945020c656e41decfba509a69bbff9f211f.

Access here: https://mattermost-webapp-pr-5105.test.mattermost.cloud

jgilliam17

Thank you @haydenhw

Tested, looks good to merge.

Tested around Change Channel URL modal, verified errors as expected.

mattermod · 2020-04-03T20:54:40Z

Test server destroyed

haydenhw closed this Mar 23, 2020

haydenhw deleted the MM-18953 branch March 23, 2020 06:21

haydenhw restored the MM-18953 branch March 23, 2020 07:00

[MM-18953] Migrate 'components/change_url_modal' and tests to TypeScript

5ce7cfe

haydenhw reopened this Mar 23, 2020

haydenhw force-pushed the MM-18953 branch from 8557f2b to 5ce7cfe Compare March 23, 2020 08:05

devinbinnie requested a review from a team March 23, 2020 13:40

ghost requested review from cpoile and hahmadia and removed request for a team March 23, 2020 13:40

devinbinnie requested a review from jgilliam17 March 23, 2020 13:40

hahmadia added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Mar 23, 2020

haydenhw commented Mar 23, 2020

View reviewed changes

cpoile suggested changes Mar 25, 2020

View reviewed changes

hahmadia suggested changes Mar 25, 2020

View reviewed changes

mattermod and others added 3 commits March 25, 2020 11:39

Merge branch 'master' into MM-18953

1ea5e7e

[MM-18953] Typescript fixes for change_url_modal component

96ca50c

* Add TS conditionals to optional props * Change this.refs.urlInput.value --> this.urlInput?.current?.value * Change JSX.Element --> React.ReactNode * Remove Props type export

Merge branch 'master' of github.com:mattermost/mattermost-webapp into…

cff5d83

… MM-18953

haydenhw commented Mar 27, 2020

View reviewed changes

hahmadia requested review from cpoile and hahmadia March 27, 2020 14:12

hahmadia approved these changes Mar 27, 2020

View reviewed changes

cpoile approved these changes Mar 29, 2020

View reviewed changes

cpoile removed the 2: Dev Review Requires review by a core commiter label Mar 29, 2020

jgilliam17 added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Mar 31, 2020

Merge branch 'master' into MM-18953

597f994

jgilliam17 approved these changes Apr 3, 2020

View reviewed changes

jgilliam17 added QA Review Done and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Apr 3, 2020

hahmadia added the 4: Reviews Complete All reviewers have approved the pull request label Apr 3, 2020

hahmadia merged commit f7fae46 into mattermost:master Apr 3, 2020

hanzei added this to the v5.24.0 milestone Apr 3, 2020

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Apr 6, 2020

fmunshi mentioned this pull request Apr 17, 2020

[MM-23727] Fix validation for channel url when renaming channel #5263

Merged

jgilliam17 added the Tests/Done Release tests have been written label May 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-18953] Migrate 'components/change_url_modal' to TypeScript #5105

[MM-18953] Migrate 'components/change_url_modal' to TypeScript #5105

haydenhw commented Mar 23, 2020

haydenhw Mar 23, 2020

cpoile Mar 25, 2020

haydenhw Mar 23, 2020

cpoile Mar 25, 2020

hahmadia Mar 25, 2020

hahmadia Mar 26, 2020

hahmadia Mar 26, 2020

haydenhw Mar 27, 2020

cpoile commented Mar 25, 2020

devinbinnie commented Mar 25, 2020

cpoile commented Mar 25, 2020

cpoile left a comment

cpoile Mar 25, 2020

cpoile Mar 25, 2020

hahmadia Mar 25, 2020

haydenhw Mar 27, 2020

cpoile Mar 25, 2020

cpoile Mar 25, 2020

hahmadia left a comment

hahmadia Mar 25, 2020

hahmadia Mar 25, 2020

hahmadia commented Mar 25, 2020

haydenhw Mar 27, 2020 •

edited

Loading

hahmadia left a comment

hahmadia commented Apr 1, 2020

mattermod commented Apr 1, 2020

jgilliam17 left a comment

mattermod commented Apr 3, 2020


		wrapper.instance().onSubmit({preventDefault: jest.fn()});
		const instance = wrapper.instance() as ChangeURLModal;

	serverError: JSX.Element \| string \| null;
	serverError?: React.ReactNode;

[MM-18953] Migrate 'components/change_url_modal' to TypeScript #5105

[MM-18953] Migrate 'components/change_url_modal' to TypeScript #5105

Conversation

haydenhw commented Mar 23, 2020

Summary

Ticket Link

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

cpoile commented Mar 25, 2020

devinbinnie commented Mar 25, 2020

cpoile commented Mar 25, 2020

cpoile 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hahmadia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hahmadia commented Mar 25, 2020

haydenhw Mar 27, 2020 • edited Loading

Choose a reason for hiding this comment

hahmadia left a comment

Choose a reason for hiding this comment

hahmadia commented Apr 1, 2020

mattermod commented Apr 1, 2020

jgilliam17 left a comment

Choose a reason for hiding this comment

mattermod commented Apr 3, 2020

haydenhw Mar 27, 2020 •

edited

Loading