Skip to content

Update .npmrc approach and locked dependencies #46601

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

Merged
merged 2 commits into from
Feb 16, 2023

Conversation

dougbu
Copy link
Member

@dougbu dougbu commented Feb 13, 2023

  • use just one .npmrc file
  • enforce resolution from correct registry in all yarn.lock files
  • bump all resolved dependencies to latest compatible versions
    • re-resolve everything but don't change any package.json files

@dougbu dougbu marked this pull request as ready for review February 13, 2023 18:33
@BrennanConroy
Copy link
Member

  • use just one .npmrc file

How does that work? What is telling npm/yarn to look ~5 folders up to find the .npmrc file?

@dougbu
Copy link
Member Author

dougbu commented Feb 13, 2023

This PR should reduce maintenance costs because (a) there's only one .npmrc file to change if someone wants to use a different registry locally (ignoring our lock files of course) and (b) the always-auth setting should (though supposedly no longer supported) allow local updates of the lock files to Just Work:tm: for authenticated users.

The CodeCheck.ps1 changes avoid checking in lock files using anything other than the dotnet-public-npm feed. @mmitche should something like that be part of a regular Arcade-ified build❔

@dotnet/aspnet-blazor-eng, @BrennanConroy, @halter73 the yarn.lock file changes are optional and mostly happened because I wanted to confirm always-auth was working as I expected. Please let me know if you would prefer I reverted some or all of those changes.

@dougbu
Copy link
Member Author

dougbu commented Feb 13, 2023

  • use just one .npmrc file

How does that work? What is telling npm/yarn to look ~5 folders up to find the .npmrc file?

I read yarn docs (somewhere) informing the world that it looked in parent directories for the .yarnrc file. Made sense it would also look up for a .npmrc file. I believe my local testing (which should have added at least a few packages / versions to dotnet-public-npm) shows this works. If there's another thing to test, I'm game…

@BrennanConroy
Copy link
Member

Ok cool 👍 just checking

Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

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

LGTM other than the one nit

@BrennanConroy
Copy link
Member

@dougbu
Copy link
Member Author

dougbu commented Feb 14, 2023

Remove https://github.com/dotnet/aspnetcore/blob/main/.yarnrc?

Great idea❕ I did that and confirmed I ingested at least a few packages w/ that change in place. Went through and bumped all versions again to make sure things are still working (defence in depth). I'll push those changes (plus the two updated src/Components/Web.JS/dist/Release/blazor.*.js files) after a local build.

See https://dev.azure.com/dnceng/public/_artifacts/feed/dotnet-public-npm/Npm/@typescript-eslint%2Fparser/overview/5.52.0 for confirmation I grabbed a new package version a couple of hours ago.

@dougbu
Copy link
Member Author

dougbu commented Feb 14, 2023

Anyone familiar w/ the following error❔ I' don't see problems in CI builds but it happened a couple of times in local builds when testing corner cases on this branch. Likely not important but I'd like to confirm…

...\eng\targets\Npm.Common.targets(45,5): error : "karma-sauce-launcher > webdriverio > @types/puppete
er-core > @types/puppeteer > puppeteer > puppeteer-core > [email protected]" has unmet peer dependency "mitt@*". [...\src\SignalR\clients\ts\FunctionalTests\SignalR.Npm.FunctionalTests.npmproj]

@dougbu dougbu force-pushed the dougbu/central.npmrc branch from cb1d241 to 1a4ef51 Compare February 14, 2023 05:23
@dougbu
Copy link
Member Author

dougbu commented Feb 14, 2023

Everything looks resolved and my local testing went well. I'll merge this tomorrow morning unless I get further comments in the meantime.

@dougbu
Copy link
Member Author

dougbu commented Feb 15, 2023

Not merging this immediately because the aspnetcore-components-e2e pipeline is failing w/ the error I occasionally saw locally and mentioned above. See https://dev.azure.com/dnceng-public/public/_build/results?buildId=170887&view=logs&j=fe94c0c9-bb8c-5d6f-3b51-887173cc2f5c&t=7877c2d9-a5b1-59a3-dc2b-0d7e9d9db055&l=674 or

/home/vsts/work/1/s/eng/targets/Npm.Common.targets(45,5): error : "karma-sauce-launcher > webdriverio > @types/puppeteer-core > @types/puppeteer > puppeteer > puppeteer-core > [email protected]" has unmet peer dependency "mitt@*". [/home/vsts/work/1/s/src/SignalR/clients/ts/FunctionalTests/SignalR.Npm.FunctionalTests.npmproj]

One odd thing is https://www.npmjs.com/package/chromium-bidi?activeTab=dependencies shows mitt as a dev dependency. No idea why that matters in dependent code.

Another odd thing is this message shows up only as a buried warning in aspnetcore-ci jobs. Why was it an error in only the one pipeline❔

In any case, I did some searching and found the solution is found in the peerDependencies section of the relevant package.json file. Will update this PR after another local build (because I had to rebase after Tanay & @wtgodbe's #45682), likely tomorrow morning.

/cc @dotnet/aspnet-blazor-eng @BrennanConroy @halter73

- use just one .npmrc file
- remove .yarnrc file
  - unnecessary since `npm` and `yarn` will both use the .npmrc file
    - (I originally created the .yarnrc when trying fixes for Dependabot updates)
- enforce resolution from correct registry in all yarn.lock files
  - add line number for new CodeCheck errors
    - update `LogError` to take new `-LineNumber` parameter
- bump all resolved dependencies to latest compatible versions
  - re-resolve everything but don't change any package.json files
- update generated JS files
  - changes likely the result of `npm` package version bumps
- nit: ignore projects found after building e.g. for project template tests
  - previous code seemed designed for clean (CI) environments
- avoid '... [email protected]" has unmet peer dependency "mitt@*"' errors and warnings
@dougbu dougbu force-pushed the dougbu/central.npmrc branch from 1a4ef51 to 059bbc1 Compare February 16, 2023 20:34
@dougbu
Copy link
Member Author

dougbu commented Feb 16, 2023

In any case, I did some searching and found the solution is found in the peerDependencies section of the relevant package.json file. Will update this PR after another local build (because I had to rebase after Tanay & @wtgodbe's #45682), likely tomorrow morning.

This took longer than expected because peerDependencies didn't work. Switched to devDependencies for the new mitt requirement and we're all good locally. I've enabled auto-merge (squash)…

@dougbu
Copy link
Member Author

dougbu commented Feb 16, 2023

Actually, I'm building locally to see if the generated JS files need another update. Will enable auto-merge if my local build comes back clean in that area.

@dougbu dougbu enabled auto-merge (squash) February 16, 2023 20:54
@dougbu dougbu merged commit 9bb6ab7 into dotnet:main Feb 16, 2023
@dougbu dougbu deleted the dougbu/central.npmrc branch February 16, 2023 22:09
@ghost ghost added this to the 8.0-preview2 milestone Feb 16, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants