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

Add Step To Verify Yarn and NPM Packages Are In Sync #617

Merged
merged 17 commits into from
Oct 10, 2022

Conversation

nagilson
Copy link
Member

@nagilson nagilson commented Oct 6, 2022

MERGE AFTER #616

With the new registry changes, we need to have a yarn.lock, otherwise yarn will update dependencies differently from npm and fail with auth errors. The reason we use two package managers is because of unsupported features in VSCE with npm so we can't only use npm, and AzDo does not support yarn, so we cannot only use yarn. We don't want the two to get out of sync, so this adds a checker that checks the diff of a PR to see if any changes made in npm locks are reflected in yarn locks.

Limitations:

  • It doesn't sync them for you, you need to sync them after it fails the pipeline.
  • If someone updates the versions incorrectly but both have been updated, the pipeline will not fail. Extracting the version from the package-lock is not super trivial, though also not that hard to do later.
  • The target branch is currently fetched from origin/, in a forked branch they must have a copy of the target branch, otherwise the sync will be more aggressive. We could add a custom remote in the script instead of calling into origin/ but that would break PRs on top of forks

@nagilson nagilson changed the base branch from nagilson-refresh to main October 6, 2022 22:16
@nagilson nagilson changed the title Add new python step to pipeline Add Step To Verify Yarn and NPM Packages Are In Sync Oct 6, 2022
@nagilson nagilson added dependencies Pull requests that update a dependency file install scripts labels Oct 6, 2022
@NTaylorMullen
Copy link
Contributor

NTaylorMullen commented Oct 10, 2022

Can you refresh me on why we have both npm and yarn ?

@NTaylorMullen
Copy link
Contributor

Forget me, I didn't read yopur description before asking like a dumb dumb: The reason we use two package managers is because of unsupported features in VSCE with npm so we can't only use npm, and AzDo does not support yarn, so we cannot only use yarn.

pool:
vmImage: 'windows-latest'
steps:
- task: UsePythonVersion@0
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, out of curiosity why use Python? Familiarity I assume?

Copy link
Member Author

@nagilson nagilson Oct 10, 2022

Choose a reason for hiding this comment

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

Yea, familiarity + speed of development time. Writing this in bash sounds... not fun. C# is an option though it's also not really in the repository much so either way it's an introduced new onboarding cost. My hope is someday VSCE will support these parts of npm and this script can be eradicated, or AzDo will support yarn and we can eliminate NPM, but since their issues for NPM have been open for a few years now, I don't expect it to happen anytime soon

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes total sense!

@dbaeumer is there someone we can chat with to better understand VSCE expectations / details around yarn / npm integrations?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for linking, I did reach out to Joao. Here are the relevant issues in VSCE's repo microsoft/vscode-vsce#308
microsoft/vscode-vsce#580
microsoft/vscode-vsce#300
microsoft/vscode-vsce#381

Choose a reason for hiding this comment

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

@NTaylorMullen the person to talk to is @joaomoreno

Choose a reason for hiding this comment

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

My hope is someday VSCE will support these parts of npm

What specific features are you referring to?

Copy link
Member Author

Choose a reason for hiding this comment

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

What specific features?

Thanks for following up (offline as well a while back.) All of the above are relevant but here are the main 2: microsoft/vscode-vsce#308 and microsoft/vscode-vsce#580

@nagilson nagilson merged commit 7bc0fb9 into main Oct 10, 2022
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 install scripts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants