Skip to content

WIP: set engineStrict config when package manager is pnpm - #1344

Draft
pluxain wants to merge 7 commits into
sveltejs:mainfrom
pluxain:f/engine-strict-pnpm
Draft

pluxain wants to merge 7 commits into
sveltejs:mainfrom
pluxain:f/engine-strict-pnpm

Conversation

@pluxain

@pluxain pluxain commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Closes #1341

Description

Following the allowBuilds approach, we add the engineStrict config to the pnpm-workspace.yml if it is not already set.

Checklist

  • Update snapshots (if applicable)
  • Add a changeset (if applicable)
  • Allow maintainers to edit this PR
  • I care about what I'm doing, no matter the tool I use (Notepad, Sublime, VSCode, AI...)

@pkg-svelte-dev

pkg-svelte-dev Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Install the latest version of sv from b0bf634:

pnx https://pkg.svelte.dev/sv/c/b0bf634c2084b7aea2783128a3ad396422de0f7f create

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

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 Sep 29, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: b0bf634

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

@pluxain

pluxain commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Hi, this is my first attempt to contribute to the project. I followed what was done for allowBuilds.

I miss some things though: for instance I could not run the branch locally to see if the generated project output was correct.

I also do not know if I should regenerate the snapshots or the changeset.

Any guidance is welcome! Cheers!

Comment thread packages/sv/src/cli/create.ts
@pluxain
pluxain marked this pull request as draft September 30, 2026 07:27
@sacrosanctic

sacrosanctic commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

allowBuilds is public because add-on developers would use it for their own app. But I don't see why this needs to be public. It's just for scaffolding brand now templates.

As an aside, I didn't want to fix it cause the whole create logic should be rewritten which would make this a no brainer fix.

@manuel3108

Copy link
Copy Markdown
Member

Hi, this is my first attempt to contribute to the project

Thanks for helping out!

I miss some things though: for instance I could not run the branch locally to see if the generated project output was correct.

What exactly was not working? Im pretty sure we are just missing something CONTRIBUTING.md

Snapshots should indeed be regenerated. Also a changeset is required here, since this is a public facing change, otherwise no new release will be triggered.

@pluxain

pluxain commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

What exactly was not working? Im pretty sure we are just missing something CONTRIBUTING.md

CONTRIBUTING.md says:

## Build and run

To build the project and all packages. Run the 'build' script:

```sh
# from root of project
pnpm build
```

This outputs into /packages/PACKAGE/dist/.

Run the 'cli' package:

```sh
pnpm sv
pnpm sv create
pnpm sv add
pnpm sv migrate
pnpm sv check
pnpm sv help
```

So I thought I should buid the project locally and run pnpm sv create but then I get this output:

[ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL] Command "sv" not found

@pluxain

pluxain commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

Also, when I try to regenerate the snapshots, using this (taken from the CONTRIBUTING.md file again):

pnpm build && pnpm test --project cli --update all

I receive this output and it runs forever:

$ vitest run --silent --project cli --update all

 RUN  v4.1.8 /home/calou/dev/@sveltejs/cli

│
■  The --tasks values all and prerequisite cannot be combined with other tasks.
│
└  Operation failed.

│
■  The --tasks values all and prerequisite cannot be combined with other tasks.
│
└  Operation failed.

│
■  Unknown migration task: missing
│  Available tasks: svelte-config, env-vars
│
└  Operation failed.

 ✓  cli  tests/migrate.ts (13 tests) 19ms

 ❯  cli  tests/cli.ts 2/5

 Test Files 1 passed (2)
      Tests 15 passed (18)
   Start at 14:55:24
   Duration 138.01s

@sacrosanctic

Copy link
Copy Markdown
Contributor

That's a visual bug. I fixed it in the other PR.

@sacrosanctic

Copy link
Copy Markdown
Contributor

[ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL] Command "sv" not found

This should work. What version of pnpm are you on and what folder did you run it in. Should be pnpm 11 and root respectively.

@pluxain

pluxain commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

run at the root of the project:

 pnpm --version

gives

11.27.1

(I removed the warning about packageManager and devEngines.packageManager specifying a different version.

But anywhere else on my machine:

 pnpm --version

yields

12.4.2

@jycouet

jycouet commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

TBH, I never use pnpm sv but always pnpm build && node ./packages/sv/dist/bin.mjs (from repo root).
Like this I'm using "raw node". I also had issues in the past with pnpm sv with a global installation.

Maybe we should tweak this in the readme? WDYT?

@sacrosanctic

Copy link
Copy Markdown
Contributor

pnpm bootstraps node to the correct version we pinned. Which also means pnpm sv works whether you have a different version installed in global or none installed at all.

I think pnpm 11 works a lot better so I rather triage the problem.

Comment thread packages/sv-utils/src/pnpm-internals.ts Outdated
Comment thread packages/sv-utils/src/pnpm.ts Outdated
Comment thread packages/sv/src/cli/create.ts
@pluxain

pluxain commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

allowBuilds is public because add-on developers would use it for their own app. But I don't see why this needs to be public. It's just for scaffolding brand now templates.

As an aside, I didn't want to fix it cause the whole create logic should be rewritten which would make this a no brainer fix.

Hi Scott,

Do you mean in this case everything should happen in addEngineStrictIfPnpm ?

I tried to follow the approach for allowBuilds as I wasn't sure of why it is structure like that.

As I stated in my comment, I would rather have the engineStrict added in the pnpm-workspace.yml with allowBuilds in one go to avoid writing the file over. I do not want to have a misleading name addAllowBuildsIfPnpm which also have a side effect on engineStrict config. What do you say ?

@sacrosanctic

sacrosanctic commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Do you mean in this case everything should happen in addEngineStrictIfPnpm ?

Yes, keep it private.

one go to avoid writing the file over

You can write to the same file multiple times. We do it all the time in this codebase.

@manuel3108
manuel3108 changed the base branch from version-1 to main October 3, 2026 08:04
@pluxain
pluxain force-pushed the f/engine-strict-pnpm branch from be05ed9 to ad3bd63 Compare October 5, 2026 07:25
Comment thread .changeset/early-kiwis-drum.md Outdated
Comment thread packages/sv-utils/src/pnpm-internals.ts Outdated
export function writeEngineStrict(): TransformFn {
return transforms.yaml(({ data }) => {
const existing = data.get('engineStrict');
// if not set, set to 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.

Suggested change
// if not set, set to true

Comment thread packages/sv-utils/src/pnpm.ts Outdated
Comment on lines +34 to +41

/**
* Returns a TransformFn for `pnpm-workspace.yaml` that adds `engineStrict` config
* if not defined already
*/
export function engineStrict(): TransformFn {
return writeEngineStrict();
}

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.

remove from sv-utils

packageManager: AgentName | null | undefined;
}): void {
const { cwd, packageManager } = options;
if (packageManager !== 'pnpm') return;

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.

remove the config from .npmrc

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, .npmrc files are present in templates and only contain engine-strict=true.

Maybe we can remove the .npmrc completely when the package manager is pnpm ? @see https://pnpm.io/blog/releases/11.0#npmrc-is-authregistry-only

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.

That is fine. Something along the lines of: remove prop, if file is empty, delete file. In case we add other props in the future.

Comment thread .changeset/early-kiwis-drum.md Outdated
Co-authored-by: Scott Wu <sw@scottwu.ca>
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.

Set engine-strict config in pnpm as well

4 participants