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

refactor: upgrade components #1677

Draft
wants to merge 3 commits into
base: alpha
Choose a base branch
from

Conversation

romanbsd
Copy link

@romanbsd romanbsd commented Apr 6, 2021

This updates the dependencies to the latest versions and fixes few bugs and warnings.
I switched from npm to yarn since npm was not able to resolve dependencies.
It needs testing in the graphql area.

@ghost
Copy link

ghost commented Apr 6, 2021

Warnings
⚠️ Please add a changelog entry for your changes.

Generated by 🚫 dangerJS

Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Great job!

.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Good to hear it works with npm and we don't need yarn after all.

When you do the changes, especially removing the range operators, you may want to do that in batches, delete package-lock and node_modules and run npm i in between, because that may be what has caused the install to fail previously.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/components/Loader/Loader.example.js Show resolved Hide resolved
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

It is somewhat surprising that despite the update of so many dependencies, sometimes by several major versions, virtually no code changes were necessary.

The CI will easily pass because Parse Dashboard does not seem to be tested for functionality, so this seems suspicious to me. I think it would be good to test this thoroughly by hand before merging.

@mtrezza
Copy link
Member

mtrezza commented Apr 9, 2021

I switched from npm to yarn since npm was not able to resolve dependencies

@romanbsd Did you use npm 7 when you had this issue, and now it is working because you use npm 6?

@mtrezza
Copy link
Member

mtrezza commented Aug 23, 2021

What should we do with this PR? I'm concerned about the many major version upgrades in the dependencies without any significant code changes in dashboard. It's possible that we are breaking something and don't notice it, unless we go through the dashboard and test some functionality I guess.

@romanbsd
Copy link
Author

It's up to you what to do about this PR. For me it was the first step before trying to modernize the project and bring the components to the latest recommended practices. It was working for me (tm), but this definitely needs more testing.

@mtrezza
Copy link
Member

mtrezza commented Aug 24, 2021

It this PR in response to #1679?

@romanbsd
Copy link
Author

This PR is a preparation for #1761

@mtrezza
Copy link
Member

mtrezza commented Aug 24, 2021

Then how is this PR related to #1679, which seems to be a more fundamental change. Does it make sense to do the changes in this PR, or would that become obsolete anyway with #1679?

@romanbsd
Copy link
Author

Besides upgrading the components, it also has one bugfix:
https://github.com/parse-community/parse-dashboard/pull/1677/files#diff-f4ba6f461663e0326e0cd8e081894cc4b977bacd715975017d1c09a41c76fa02L11

@romanbsd
Copy link
Author

It's related, as it's a first step for 1679. But this PR can be reviewed in isolation. It's not related per se, and was opened before.

@mtrezza mtrezza changed the title Update components to the latest version. refactor: upgrade components Sep 19, 2021
@mtrezza
Copy link
Member

mtrezza commented Sep 19, 2021

I suppose we could proceed this way:

  1. merge master into this and resolve conflicts
  2. test out the dashboard thoroughly by hand, since there are some major version upgrades and we don't have UI tests

@romanbsd would you want to do (1)? Then I would take care of (2).

@mtrezza mtrezza mentioned this pull request Oct 20, 2021
5 tasks
@mtrezza mtrezza marked this pull request as draft October 20, 2021 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants