-
Notifications
You must be signed in to change notification settings - Fork 327
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
chore: remove node-fetch from app code #8178
Conversation
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
@@ -38,7 +37,8 @@ const updateUserProfile = async ( | |||
|
|||
if (validUpdatedUser.picture && validUpdatedUser.picture.endsWith('.svg')) { | |||
const res = await fetch(validUpdatedUser.picture) | |||
const buffer = await res.buffer() | |||
const arrayBuffer = await res.arrayBuffer() |
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.
this is the only real change
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.
+1 That piece of code is unused. Apparently we only have one place where we call updateUserProfile
and there we only update the name:
parabol/packages/client/modules/userDashboard/components/UserSettingsForm/UserSettingsForm.tsx
Line 78 in a3c4856
UpdateUserProfileMutation(atmosphere, {updatedUser: {preferredName}}, {onError, onCompleted}) |
We seem to only upload images via
uploadUserImage
where we convert SVGs to PNG firstconst [normalExt, normalBuffer] = await normalizeAvatarUpload(validExt, validBuffer) |
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.
right you are! we got some good spaghetti code going on there.
shall i remove that .svg
conditional clause in this PR or should we save it for later? OK with either option.
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.
Let's leave it for later and then let's remove the picture
input from this mutation completely.
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.
Issue made! #8207
@@ -55,7 +55,7 @@ | |||
"eslint-plugin-emotion": "^10.0.14", | |||
"eslint-plugin-react": "^7.16.0", | |||
"eslint-plugin-react-hooks": "^1.6.1", | |||
"jest": "^27.0.6", | |||
"jest": "^29.5.0", |
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.
native fetch
isn't available to jest until v28 so i bumped to latest
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
@@ -38,7 +37,8 @@ const updateUserProfile = async ( | |||
|
|||
if (validUpdatedUser.picture && validUpdatedUser.picture.endsWith('.svg')) { | |||
const res = await fetch(validUpdatedUser.picture) | |||
const buffer = await res.buffer() | |||
const arrayBuffer = await res.arrayBuffer() |
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.
+1 That piece of code is unused. Apparently we only have one place where we call updateUserProfile
and there we only update the name:
parabol/packages/client/modules/userDashboard/components/UserSettingsForm/UserSettingsForm.tsx
Line 78 in a3c4856
UpdateUserProfileMutation(atmosphere, {updatedUser: {preferredName}}, {onError, onCompleted}) |
We seem to only upload images via
uploadUserImage
where we convert SVGs to PNG firstconst [normalExt, normalBuffer] = await normalizeAvatarUpload(validExt, validBuffer) |
Signed-off-by: Matt Krick <matt.krick@gmail.com>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
Description
.buffer
, which doesn't exist on native fetchTesting scenarios