Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

[Build, Compiler, Refactor] Upgrade typescript and add optional-chaining and nullish-coalescing #4604

Merged
merged 5 commits into from
Jan 16, 2020
Merged

[Build, Compiler, Refactor] Upgrade typescript and add optional-chaining and nullish-coalescing #4604

merged 5 commits into from
Jan 16, 2020

Conversation

calebroseland
Copy link
Member

@calebroseland calebroseland commented Jan 1, 2020 •
edited
Loading

Summary

This PR introduces JS/TS features Optional Chaining (?.) and Nullish Coalescing (??). I believe the timing is appropriate with the recent alignment between TypeScript 3.7.x and TC39 proposals reaching stages 4 and 3.

  • Upgrades TypeScript to 3.7.4, stable/"final" since 3.7.2. (This handles the .ts/.tsx side of things.)
  • Adds Babel plugins @babel/plugin-proposal-optional-chaining and @babel/plugin-proposal-nullish-coalescing-operator. (This handles the .js/.jsx side of things.)
  • Adds coverage folder to .gitignore to make working with npm run test:coverage easier.

Also included is an introductory refactoring to show how these new operators could be helpful. (See changes to utils/youtube.js.)

NOTE

If your editor (e.g. VS Code) is not recognizing the newer syntax, you may need to delete node_modules and/or re-select your TypeScript version.

Ticket Link

None

Related Pull Requests

None

Screenshots

N/A, no UI/UX changes.

@calebroseland calebroseland changed the title Add efactor youtube util handleLinkTime [Build, Compiler, Refactor] Upgrade typescript and add nullish-coalescing and optional-chaining Jan 1, 2020
@calebroseland calebroseland changed the title [Build, Compiler, Refactor] Upgrade typescript and add nullish-coalescing and optional-chaining [Build, Compiler, Refactor] Upgrade typescript and add optional-chaining and nullish-coalescing Jan 1, 2020
@jasonblais jasonblais added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Jan 2, 2020
@crspeller crspeller requested review from hmhealey and removed request for crspeller January 2, 2020 15:25
hmhealey
hmhealey approved these changes Jan 2, 2020
View reviewed changes
Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

Great change!

There's a small chance that the changes to utils/youtube.js conflict with another PR, but I can't find any PR that makes changes to that file, so I might be misremembering there.

saturninoabril
saturninoabril suggested changes Jan 3, 2020
View reviewed changes
Copy link
Member

@saturninoabril saturninoabril left a comment

Choose a reason for hiding this comment

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

Looks good to me, except for minor request. Also, there's codemod available online in case you'll want to fully migrate like https://github.com/villesau/optional-chaining-codemod

package.json Outdated Show resolved Hide resolved
saturninoabril
saturninoabril approved these changes Jan 4, 2020
View reviewed changes
Copy link
Member

@saturninoabril saturninoabril left a comment

Choose a reason for hiding this comment

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

Thanks @calebroseland, LGTM!

@saturninoabril saturninoabril added QA Review Done and removed 3: QA Review Requires review by a QA tester labels Jan 4, 2020
@hanzei
Copy link
Contributor

hanzei commented Jan 13, 2020

@wiersgallak @jaydeland Would you please review this PR soon? It should be great if developers could use this feature soon.

@jaydeland
Copy link
Contributor

@wiersgallak @jaydeland Would you please review this PR soon? It should be great if developers could use this feature soon.

@hanzei - I am not sure what value I add to this review from a DevOps/Infra standpoint.

@hmhealey hmhealey requested review from jespino and removed request for jaydeland January 14, 2020 21:51
@hmhealey
Copy link
Member

Re-assigning the second dev review to @jespino since he's more familiar with the TypeScript migration.

wiersgallak
wiersgallak approved these changes Jan 14, 2020
View reviewed changes
Copy link
Contributor

@wiersgallak wiersgallak left a comment

Choose a reason for hiding this comment

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

Created a ticket to track the follow-up QA work: https://mattermost.atlassian.net/browse/MM-21766 FYI @lindy65. Since web-app, assigned to the apps team. Otherwise, all good from my side.

@wiersgallak wiersgallak removed the 1: PM Review Requires review by a product manager label Jan 14, 2020
jespino
jespino approved these changes Jan 16, 2020
View reviewed changes
Copy link
Member

@jespino jespino left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks @calebroseland really nice PR. 🎉

@jespino jespino removed the 2: Dev Review Requires review by a core commiter label Jan 16, 2020
@jespino jespino added the 4: Reviews Complete All reviewers have approved the pull request label Jan 16, 2020
@jespino jespino self-assigned this Jan 16, 2020
@jespino
Copy link
Member

jespino commented Jan 16, 2020

/update-branch

@jespino jespino merged commit 49441fa into mattermost:master Jan 16, 2020
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jan 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jespino jespino jespino approved these changes

@hmhealey hmhealey hmhealey approved these changes

@saturninoabril saturninoabril saturninoabril approved these changes

@wiersgallak wiersgallak wiersgallak approved these changes

Assignees

@jespino jespino

Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation QA Review Done
Projects
None yet
Milestone
No milestone
10 participants
@calebroseland @hanzei @jaydeland @hmhealey @jespino @saturninoabril @wiersgallak @jasonblais @amyblais @mattermod