-
Notifications
You must be signed in to change notification settings - Fork 848
Add .npmrc next to package.json and add lockfile for PublishAIEvaluationReport #7108
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
base: main
Are you sure you want to change the base?
Conversation
The .npmrc is not transitively looked up in parent directories like other config files, it needs to be next to the package.json Also remove the always-auth entry since it is deprecated/unused in npm and will be removed: actions/setup-node#1305
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves npm configuration by placing .npmrc files directly adjacent to package.json files, as npm does not transitively search parent directories for .npmrc like it does for other configuration files. The PR also removes the deprecated always-auth setting from the root configuration.
Key changes:
- Added
.npmrcfiles next topackage.jsonin two npm workspace locations - Removed deprecated
always-auth=truesetting from root.npmrc
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/ProjectTemplates/.npmrc |
New npm registry configuration for the project templates workspace |
src/Libraries/Microsoft.Extensions.AI.Evaluation.Reporting/TypeScript/.npmrc |
New npm registry configuration for the AI evaluation reporting TypeScript workspace |
.npmrc |
Removed deprecated always-auth setting while keeping registry configuration |
|
@peterwald Could you please take a quick look - looks like the .NET part of the build is failing |
…enticated developer
|
@shyamnamboodiripad the failure was actually related, I found that you ran Using One wrinkle is that whenever you actually want to bump versions you need to do that locally with an authenticated user for the dotnet-public-npm feed so that it ingests the packages. And one more issue is that npm by default only fetches the optional dependencies for e.g. esbuild or rollup for the current platform (e.g. @rollup/rollup-win32-x64-msvc), so if you restore on Windows it will only ingest those matching packages which won't be enough on Linux/Mac. aspnetcore uses this script to manually fetch optionalDependencies: https://github.com/dotnet/aspnetcore/blob/a9aaa320f1c4c771b2dee8c000409a5f04397339/eng/scripts/update-npm-dependencies.ps1#L45-L63, I used a modified version of that to get both rollup and esbuild. I can port it here if you want. |
|
FYI @ericstj @jeffhandley this will probably conflict with #7113 |
|
Thank you @akoeplinger.
I may be wrong, but I think it was already the case that we had to update the lock file manually when updating packages. (For example, I remember some of the dependabot created PRs for bumping dependency versions would fail without cloning the bot's branch and running the build locally to update the lock file...) So, this sounds reasonable.
Ah interesting... Is this a limitation only with
Yes, given the above limitation, it seems like a good idea to port it. Thanks! That said I would defer to @peterwald who is more familiar with the build authoring and publishing for the JavaScript assets in the evaluation reporting library - especially the changes in the Azure DevOps extension that are also happening in this PR. @peterwald Could you please review? |
Thanks @shyamnamboodiripad. These changes look good to me. |
Yes this was already the case with |
The .npmrc is not transitively looked up in parent directories like other config files, it needs to be next to the package.json
Add a lockfile for PublishAIEvaluationReport and make sure we're using
npm ciinstead ofnpm installeverywhere so that we only restore the dependencies but not upgrade them.Also remove the always-auth entry since it is deprecated/unused in npm and will be removed: actions/setup-node#1305
Microsoft Reviewers: Open in CodeFlow