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

yarn upgrade && yarn format-all #2779

Merged
merged 7 commits into from
Sep 6, 2021
Merged

yarn upgrade && yarn format-all #2779

merged 7 commits into from
Sep 6, 2021

Conversation

casperdcl
Copy link
Contributor

@casperdcl casperdcl commented Sep 1, 2021

Using:

Then:

  • yarn format-all: 10 files changed, 57 insertions(+), 60 deletions(-)
  • npm update: updates 10 package versions in package.json
  • yarn upgrade: 5775 insertions(+), 5160 deletions(-) in yarn.lock

Related:

@shcheklein shcheklein temporarily deployed to dvc-org-node-up-otgt393o4prqrk September 1, 2021 13:12 Inactive
@casperdcl casperdcl added the dependencies Pull requests that update a dependency file (automatic) label Sep 1, 2021
@shcheklein shcheklein temporarily deployed to dvc-org-node-up-dmoo22ctkpjhl8 September 1, 2021 19:40 Inactive
@rogermparent
Copy link
Contributor

outdated yarn.lock broke the deploy, just pushed an update

@casperdcl
Copy link
Contributor Author

casperdcl commented Sep 1, 2021

Ah so do we need to update the instructions over at https://dvc.org/doc/user-guide/contributing/docs#development-environment to remove mentions of npm and instead use yarn? We have a yarn.lock but not package-lock.json.

@shcheklein shcheklein temporarily deployed to dvc-org-node-up-dmoo22ctkpjhl8 September 1, 2021 21:38 Inactive
@rogermparent
Copy link
Contributor

Other than this one entry:

You may need to resolve dependencies at this point by running:

npm install

It seems that we already do use yarn everywhere on that page. The first mention of npm install -g yarn is just to get yarn on the system, since if a user doesn't have yarn they'll almost certainly have npm. That second one should be changed, though, since after yarn is installed the user shouldn't be running npm.

@rogermparent
Copy link
Contributor

#2785 is up for this, thanks for pointing it out!

@casperdcl
Copy link
Contributor Author

ok so can this PR be merged @rogermparent? Or would you like to fix the lint-ts stuff first?

@shcheklein
Copy link
Member

I think we need to fix the issue, CI should be clean to my mind.

@julieg18
Copy link
Contributor

julieg18 commented Sep 3, 2021

I think we need to fix the issue, CI should be clean to my mind.

@rogermparent, I can help clear the errors if needed!

@julieg18 julieg18 temporarily deployed to dvc-org-node-up-dmoo22ctkpjhl8 September 3, 2021 17:12 Inactive
@casperdcl casperdcl changed the title npm update && yarn format-all yarn upgrade && yarn format-all Sep 3, 2021
@rogermparent
Copy link
Contributor

I can help clear the errors if needed!

@julieg18 That'd be great, thanks! Feel free to ping me if you get stuck on something.

* fix errors found in Blog/Post/HeroPic and templates/blog-post.tsx
@julieg18 julieg18 temporarily deployed to dvc-org-node-up-dmoo22ctkpjhl8 September 3, 2021 19:36 Inactive
Copy link
Contributor Author

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

lgtm but I can't approve a PR I initiated

casperdcl added a commit to iterative/cml.dev that referenced this pull request Sep 3, 2021
@julieg18
Copy link
Contributor

julieg18 commented Sep 3, 2021

@rogermparent, can you review the lint fixes?

Copy link
Contributor

@rogermparent rogermparent left a comment

Choose a reason for hiding this comment

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

One nitpick which looked like it could be a logic problem, but looking at the whole file seems to be nothing but an opportunity for slightly reduced complexity to get the same practical effect.

Everything else looks fine, nice job!

src/components/Blog/Feed/Item/index.tsx Outdated Show resolved Hide resolved
casperdcl added a commit to iterative/cml.dev that referenced this pull request Sep 6, 2021
@julieg18 julieg18 temporarily deployed to dvc-org-node-up-dmoo22ctkpjhl8 September 6, 2021 12:41 Inactive
@casperdcl
Copy link
Contributor Author

casperdcl commented Sep 6, 2021

yarn lint-ts still yields 5 errors for me 🤔 nvm ignore me

@julieg18
Copy link
Contributor

julieg18 commented Sep 6, 2021

Actually the deployed site isn't working correctly for me... I'm going to try redeploying.

@casperdcl
Copy link
Contributor Author

maybe now after another yarn upgrade?

@shcheklein shcheklein temporarily deployed to dvc-org-node-up-dmoo22ctkpjhl8 September 6, 2021 14:22 Inactive
@julieg18
Copy link
Contributor

julieg18 commented Sep 6, 2021

Working now! @casperdcl, is it working for you? I think we're good to merge if so!

@casperdcl casperdcl merged commit d16ff85 into master Sep 6, 2021
@casperdcl casperdcl deleted the node-up branch September 6, 2021 14:39
julieg18 added a commit that referenced this pull request Sep 6, 2021
shcheklein pushed a commit that referenced this pull request Sep 6, 2021
@julieg18 julieg18 mentioned this pull request Sep 7, 2021
@jorgeorpinel
Copy link
Contributor

As this was rolled back, should we re-try in 2 separate PRs? One for yarn update and another (nested?) one for yarn format-all

karajan1001 pushed a commit to karajan1001/dvc.org that referenced this pull request Sep 29, 2021
karajan1001 pushed a commit to karajan1001/dvc.org that referenced this pull request Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file (automatic)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants