-
Notifications
You must be signed in to change notification settings - Fork 30
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
Bump react-scripts from 3.4.1 to 4.0.3 + fix associated tech debt #1102
Conversation
Code Climate has analyzed commit e384d8f and detected 1 issue on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR diff size of 16217 lines exceeds the maximum allowed for the inline comments feature.
Note:
|
beccf47
to
dbeb394
Compare
|
…AR_IMAGE, DEFAULT_PROFILE_PIC
Currently on: https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/CHANGELOG.md#371-2020-07-27
Latest seems to be Looks like
I tried explicitly removing the dependencies from our package files to use the version it supposedly includes by default, but I was getting all sorts of errors/issues. There used to be a There seems to be more confusion in this thread about it all as well: This even spun out a website to attempt to tell people how they can set everything up: Yet when I try and use that config without having any additional dependencies installed directly within our own
Based on facebook/create-react-app#9083 (comment) I tried deleting the entire
So I guess the next question is how can we 'clean up' the existing |
…already provides this
…lready provides them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[note] Some notes to explain various aspects of this PR to make reviewing easier.
Note also that the mass of changes to package-lock.diff
are due to react-scripts
(and all of the dependencies it wraps up) changing version (as we intended), as can be seen in dbeb394
src/components/organisms/ProfileModal/EditProfileForm/EditProfileForm.tsx
Show resolved
Hide resolved
Before
After
Note: I believe the slight increase in size shown in this bundle is due to moving some of the image assets from |
https://create-react-app.dev/docs/supported-browsers-features/
https://create-react-app.dev/docs/supported-browsers-features/#configuring-supported-browsers
https://github.com/facebook/create-react-app/blob/master/packages/react-app-polyfill/README.md This package includes polyfills for various browsers. It includes minimum requirements and commonly used language features used by Create React App projects.
We seem to already have this via
https://babeljs.io/docs/en/babel-preset-env.html
This seems to already be included via
https://babeljs.io/docs/en/babel-preset-env.html#bugfixes
https://babeljs.io/docs/en/babel-preset-env.html#usebuiltins
https://babeljs.io/docs/en/babel-preset-env.html#corejs
|
cb80ed4
to
1d578f2
Compare
1d578f2
to
b143afa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few notes/questions
const clearSearchIcon = ( | ||
<img | ||
className="NavSearchBar__clear-search" | ||
src={CloseIcon} | ||
src={navDropdownCloseIcon} | ||
alt="close button" | ||
onClick={clearSearchQuery} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be nicer to remove it & use font awesome instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially, though @mcflyok tends not to like the font awesome iconography in general I believe.
This was just renaming the variable name of existing code, not intending to change logic in this PR.
takeSeat(null, null); | ||
}; | ||
const leaveSeat = useCallback(() => { | ||
takeSeat(null, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we use null here because the implementation with undefined is not merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the implementation with undefined
out of curiosity?
I didn't seek to change any of the existing functionality in this PR, so I left it as what it was.
…ade-react-scripts
There was a problem hiding this 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 (let's see if this satisfies the CODEOWNERS rule for @sparkletown/engineering-leads)
Edit: Seemingly it doesn't, and is a long outstanding issue: https://git.luolix.topmunity/t/do-not-require-owner-approval-if-the-pull-request-is-from-an-owner/369/64
Temporarily disabled the 'require CODEOWNERS' branch protection to get around this.
Bumps react-scripts from 3.4.1 to 4.0.3.
Partially fixes sparkletown/internal-sparkle-issues#617
Partially fixes sparkletown/internal-sparkle-issues#645
Release Notes
Notes
We will probably need to land thisreact-scripts
update before we can upgradenode-sass
in Bump node-sass from 4.14.1 to 5.0.0 #1079see Bump node-sass from 4.14.1 to 5.0.0 #1079 (comment) / Replace deprecated node-sass dependency with sass #850DISABLE_NEW_JSX_TRANSFORM=true
TODO
staging
once Replace deprecated node-sass dependency with dart-sass #1103 has been merged