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

Update react-scripts and Typescript to version 4 #85

Merged
merged 13 commits into from
Feb 15, 2021
Merged

Update react-scripts and Typescript to version 4 #85

merged 13 commits into from
Feb 15, 2021

Conversation

ktrieu
Copy link
Collaborator

@ktrieu ktrieu commented Jan 3, 2021

This PR updates react-scripts and Typescript to major version 4.

The following changes were required to deal with breaking changes:

  • Uninstalling our version of eslint to avoid conflicts with the version react-scripts provides. Our eslintrc has been moved to package.json, where the docs recommend ESLint configuration is done.'
  • Various changes to deal with lint errors the above changes surfaced
  • Removing a ref hack around react-router-dom Links, since the issue we were trying to work around has been fixed.
  • A change to the errors proxy to deal with react-refresh looking for $$typeof on every export. This should be temporary until we deliver a more lasting fix to the errors proxy.
  • Setting resetMocks to false in package.json. CRA 4 changed this option to true by default which breaks jest-mock-axios. Tracking issue here.
  • Storing config data in JSON, which avoids a lint error we getting in the config.ts file, and makes the manifest generation code less fragile.

@ktrieu ktrieu requested a review from tyxchen January 3, 2021 22:35
This fixes a type error we were getting on deleting window.document.cookie, which is invalid, since that field is not optional.
@codecov
Copy link

codecov bot commented Jan 4, 2021

Codecov Report

Merging #85 (0e62412) into master (12ffe92) will decrease coverage by 0.01%.
The diff coverage is 62.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
- Coverage   59.55%   59.54%   -0.02%     
==========================================
  Files          63       63              
  Lines        1533     1535       +2     
  Branches      388      390       +2     
==========================================
+ Hits          913      914       +1     
  Misses        588      588              
- Partials       32       33       +1     
Impacted Files Coverage Δ
src/dash/EditorPage.tsx 0.00% <0.00%> (ø)
src/dash/issues/DashIssueDetail.tsx 0.00% <0.00%> (ø)
src/shared/components/ActionLink.tsx 100.00% <ø> (ø)
src/shared/components/Button.tsx 100.00% <ø> (ø)
src/shared/errors.ts 69.44% <0.00%> (-1.99%) ⬇️
src/shared/components/Loader.tsx 100.00% <100.00%> (ø)
src/shared/contexts/ToastContext.tsx 25.92% <100.00%> (ø)
src/shared/form/PasswordField.tsx 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12ffe92...c8cd48b. Read the comment docs.

ktrieu added 2 commits January 3, 2021 21:53
The client manifest generation script relies on export default being there, so we just ignore the eslint rule.
This avoids the eslint error we were getting in config.ts before, and makes the manifest generation code less fragile
@github-actions
Copy link

github-actions bot commented Jan 4, 2021

size-limit report

Path Size
build/static/js/0.95e8426b.chunk.js, build/static/js/1.3cae06b5.chunk.js, build/static/js/11.e14e7df8.chunk.js, build/static/js/12.df2dcd17.chunk.js, build/static/js/dash-admin.cbcf2bd4.chunk.js, build/static/js/dash-articles-page.c69f95d3.chunk.js, build/static/js/dash-editor-page.03a0430d.chunk.js, build/static/js/dash-issue-detail.5c3fe04b.chunk.js, build/static/js/dash-issues-page.ae086fee.chunk.js, build/static/js/dash-profile.2b73c086.chunk.js, build/static/js/main.fb8b10a7.chunk.js, build/static/js/other-reset-password.3de49483.chunk.js, build/static/js/runtime-main.e3d7044e.js, build/static/css/dash-admin.8df4d4bb.chunk.css, build/static/css/dash-articles-page.e740f83b.chunk.css, build/static/css/dash-editor-page.803ed09c.chunk.css, build/static/css/dash-issue-detail.e740f83b.chunk.css, build/static/css/dash-issues-page.604d646f.chunk.css, build/static/css/main.60abbe16.chunk.css 0 B (-100%)
build/static/js/0.111b0df3.chunk.js, build/static/js/1.6b393d1f.chunk.js, build/static/js/11.e44186f4.chunk.js, build/static/js/12.6eae009e.chunk.js, build/static/js/dash-admin.a16e1de9.chunk.js, build/static/js/dash-articles-page.2ae333de.chunk.js, build/static/js/dash-editor-page.4e596f6f.chunk.js, build/static/js/dash-issue-detail.4607afd9.chunk.js, build/static/js/dash-issues-page.b879d331.chunk.js, build/static/js/dash-profile.dcaa0a1a.chunk.js, build/static/js/main.acf7f302.chunk.js, build/static/js/other-reset-password.837c9479.chunk.js, build/static/js/runtime-main.3ab58682.js, build/static/css/dash-admin.8ea7067e.chunk.css, build/static/css/dash-articles-page.10cda842.chunk.css, build/static/css/dash-editor-page.6844e03c.chunk.css, build/static/css/dash-issue-detail.10cda842.chunk.css, build/static/css/dash-issues-page.a9adb75b.chunk.css, build/static/css/main.e14ef68a.chunk.css 296.54 KB (+100% 🔺)

Copy link
Member

@tyxchen tyxchen left a comment

Choose a reason for hiding this comment

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

Some minor formatting stuff, then should be good to go

src/config/config.json Outdated Show resolved Hide resolved
src/config/index.ts Outdated Show resolved Hide resolved
src/shared/components/Loader.tsx Outdated Show resolved Hide resolved
@ktrieu
Copy link
Collaborator Author

ktrieu commented Feb 13, 2021

Turns out I had format on save disabled in VS Code for some reason, which explains why those format issues got through.

@tyxchen
Copy link
Member

tyxchen commented Feb 14, 2021

Awesome, it should just need a rebase on master for #86 and #73 and then we're good

@tyxchen tyxchen added blocking Blocks another issue --- see issue description for details dependencies Pull requests that update a dependency file labels Feb 14, 2021
@tyxchen
Copy link
Member

tyxchen commented Feb 15, 2021

So it looks like the security check got disabled because of inactivity. It should be good to merge anyways.

@ktrieu ktrieu merged commit c36d9f5 into master Feb 15, 2021
@ktrieu ktrieu deleted the update-cra branch February 15, 2021 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking Blocks another issue --- see issue description for details dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants