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.

Sign up for GitHub

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

Jump to bottom

feat(update): pnpm update support update corepack config #7235

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat(update): pnpm update support update corepack config #7235

wants to merge 6 commits into from

Conversation

fireairforce
Copy link
Member

@fireairforce fireairforce commented Oct 22, 2023 •
edited

closes: #7134

@fireairforce
Copy link
Member Author

I will update test later, @zkochan when you are free, you can take a look and see if there are any problems with my implementation idea. There are too many data structures in plugin-command-installation. I don’t know if I am using it correctly. Thank u~

zkochan
zkochan reviewed Oct 24, 2023
View reviewed changes
packages/types/package.json Outdated
@@ -12,6 +12,7 @@
"!*.map"
],
"scripts": {
"watch": "tsc --watch",
Copy link
Member

Choose a reason for hiding this comment

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

this change is unrelated, revert it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will revert this before the pull request pass all the CI pipeline, now i need this to debug at my local.

pkg-manager/plugin-commands-installation/src/update/index.ts Outdated
const updatedManifest = project.manifest
if (updatedManifest.packageManager) {
updatedManifest.packageManager = `${updatedPackage}@${updatedVersion}`
// eslint-disable-next-line no-await-in-loop
Copy link
Member

Choose a reason for hiding this comment

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

use a Promise.all

Copy link
Member Author

Choose a reason for hiding this comment

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

done

reviewing/outdated/src/outdated.ts Show resolved Hide resolved
reviewing/plugin-commands-outdated/package.json Outdated
@@ -12,6 +12,7 @@
"node": ">=16.14"
},
"scripts": {
"start": "tsc --watch",
Copy link
Member

Choose a reason for hiding this comment

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

unrelated change

pkg-manager/plugin-commands-installation/src/update/getUpdateChoices.ts Show resolved Hide resolved
@fireairforce fireairforce force-pushed the feat-7134 branch 2 times, most recently from a84be7f to 822845b Compare October 25, 2023 13:21
fireairforce
fireairforce commented Oct 25, 2023
View reviewed changes
pkg-manager/plugin-commands-installation/test/update/interactive.ts Outdated
choices: [
headerChoice,
{
message: chalk`pnpm 8.7.0 ❯ 8.9.2 `,
Copy link
Member Author

Choose a reason for hiding this comment

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

How can i get the latest pnpm version here at my test case... otherwise we will need to update the test case here everytime pnpm publish latest version. @zkochan

Copy link
Member

Choose a reason for hiding this comment

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

could we just remove the pnpm version before checking the snapshot?
Or mock the pnpm registry endpoint with nock?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, i will update here~

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@fireairforce
Copy link
Member Author

I'm not sure the ci failed whether about my pr change:

image

@zkochan
Copy link
Member

zkochan commented Oct 26, 2023

That failure is not related to the PR

@fireairforce
Copy link
Member Author

seems the only ci failed by the network

zkochan
zkochan reviewed Oct 27, 2023
View reviewed changes
Copy link
Member

@zkochan zkochan left a comment

Choose a reason for hiding this comment

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

add changesets

pkg-manager/plugin-commands-installation/test/update/interactive.ts Outdated
@@ -11,7 +11,7 @@ import * as enquirer from 'enquirer'
jest.mock('enquirer', () => ({ prompt: jest.fn() }))

// eslint-disable-next-line
const prompt = enquirer.prompt as any
const prompt = enquirer.prompt as any;
Copy link
Member

Choose a reason for hiding this comment

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

remove the semicolon

Copy link
Member Author

Choose a reason for hiding this comment

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

done

pkg-manager/plugin-commands-installation/test/update/interactive.ts Outdated
})

prompt.mockClear()
// t.comment('update to compatible versions')
Copy link
Member

Choose a reason for hiding this comment

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

remove this

Copy link
Member Author

Choose a reason for hiding this comment

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

done

pkg-manager/plugin-commands-installation/test/update/interactive.ts
name: '[dependencies]',
message: 'dependencies',
},
])
Copy link
Member

Choose a reason for hiding this comment

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

Looks like here only the formatting changed. Revert it back.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Member Author

Choose a reason for hiding this comment

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

i run pnpm run lint --fix at here, it will change automatally.

@zkochan
Copy link
Member

zkochan commented Oct 29, 2023

The tests are failing

@fireairforce
Copy link
Member Author

changesets add done.

@fireairforce
Copy link
Member Author

Seems the only ci failed not releated to my pr.

@zkochan
Copy link
Member

zkochan commented Oct 30, 2023

Your change doesn't seem to work. I have tested it locally and the field in package.json doesn't get updated.

@fireairforce
Copy link
Member Author

uha? you test for workspace project or singe package.json project ? i have tested for singe package.json project and find it work normal

@fireairforce
Copy link
Member Author

This need to add params --interactive for pnpm update

@zkochan
Copy link
Member

zkochan commented Nov 7, 2023

Why was this closed?

@zkochan zkochan reopened this Nov 7, 2023
@fireairforce
Copy link
Member Author

sorry, i notice the user close the related issue so i think this pr is not necessary...

@zkochan
Copy link
Member

zkochan commented Nov 10, 2023

You have already implemented it, so I think we can finish it.

@Bessonov
Copy link

@fireairforce Should I reopen the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zkochan zkochan zkochan left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pnpm update should also update the packageManager setting
3 participants
@fireairforce @zkochan @Bessonov