Skip to content

Fix/package manager settings 1371 - #1396

Open
erenbati wants to merge 8 commits into
sveltejs:mainfrom
erenbati:fix/package-manager-settings-1371
Open

erenbati wants to merge 8 commits into
sveltejs:mainfrom
erenbati:fix/package-manager-settings-1371

Conversation

@erenbati

@erenbati erenbati commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Fixes #1371

Summary

  • respect save-exact=true from project/workspace .npmrc files when dependency versions are rewritten
  • respect pnpm 11 saveExact from pnpm-workspace.yaml
  • preserve existing catalog: and catalog:<name> specifiers in package.json and update the corresponding pnpm workspace catalog entry instead
  • add focused core tests for pnpm 10-style .npmrc, pnpm 11 workspace config, default catalogs, and named catalogs
  • add an sv patch changeset

Validation

  • branch is based on current upstream main at 0a668cb96081561aa0ce2abde8de25d2150a756a
  • diff is limited to the dependency-finalization logic, focused core tests, and a changeset
  • local tests/lint/check were not run because the execution environment could not resolve github.com during clone; no local pass is claimed

@pkg-svelte-dev

pkg-svelte-dev Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Install the latest version of sv from 003923b:

pnx https://pkg.svelte.dev/sv/c/003923b1d17a3b009a4090b51cfccb6864dcb5e6 create

Open in pkg.svelte.dev: https://pkg.svelte.dev/repos/cli/pr/1396

Note

This PR is from a fork. A maintainer must approve approve each commit before it can be built and installed.

@changeset-bot

changeset-bot Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 003923b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
sv Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@sacrosanctic sacrosanctic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the migration case is not handled.

Comment thread packages/sv/src/core/engine.ts Outdated
return value as YamlMapLike;
}

function isRangeWithinSafe(declared: string, requested: string): boolean {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the underlying function already has a try catch.

Comment thread packages/sv/src/core/engine.ts Outdated
let directory = path.resolve(cwd);
const root = path.resolve(workspaceRoot);

while (true) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we have walk.up()

Comment thread packages/sv/src/core/engine.ts Outdated
if (fs.existsSync(npmrc)) {
const match = fs
.readFileSync(npmrc, 'utf8')
.match(/^\s*save-exact\s*=\s*(true|false)\s*$/im);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

.npmrc is an INI-formatted file. Can just parse it as such.

Comment thread packages/sv/src/core/engine.ts Outdated
function shouldSaveExact(workspace: Workspace): boolean {
const workspaceRoot = findWorkspaceRoot(workspace.cwd);

if (workspace.packageManager === 'pnpm') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is a user selection. Which can conflict with the configs in the repo.

@sacrosanctic

Copy link
Copy Markdown
Contributor

It looks better, but still needs work. But more importantly, I'm not sure if we should be doing this at all.

@jycouet

jycouet commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

It looks better, but still needs work. But more importantly, I'm not sure if we should be doing this at all.

I'm also thinking about it... because we have also to support the other 20 packages maangers then. Let's see how far we should go.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sv migrate should respect package manager settings when updating dependency versions

3 participants