C3: Fix usage with yarn #4311

jculvey · 2023-10-31T04:07:43Z

What this PR solves / how to test:

This fixes a regression that was causing c3 to fail with web frameworks when using yarn.

Being able to version the framework creators we use is important for c3, but unfortunately yarn can't yarn create some-framework@some-particular-version. This change modifies the runFrameworkGenerator helper to use npx and spoof the npm user agent so framework creators treat it like a yarn create invocation.

It also patches the package manager reset logic to make it only run when truly needed (i.e. when there's a mismatch between installed node modules and the type of lockfile used by the desired package manager).

This also adds e2e coverage for yarn to hopefully prevent future regressions.

Author has addressed the following:

Tests
- Included
- Not necessary because:
Changeset (Changeset guidelines)
- Included
- Not necessary because:
Associated docs
- Issue(s)/PR(s):
- Not necessary because:

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

changeset-bot · 2023-10-31T04:07:46Z

🦋 Changeset detected

Latest commit: ece618d

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

This PR includes changesets to release 1 package

Name	Type
create-cloudflare	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

github-actions · 2023-10-31T04:10:35Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6789015440/npm-package-wrangler-4311

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6789015440/npm-package-wrangler-4311

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6789015440/npm-package-wrangler-4311 dev path/to/script.js

Additional artifacts:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6789015440/npm-package-miniflare-4311

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/6789015440/npm-package-cloudflare-pages-shared-4311

Note that these links will no longer work once the GitHub Actions artifact expires.

wrangler@3.15.0 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20231025.1
`workerd`	1.20231030.0	1.20231030.0
`workerd --version`	1.20231030.0	2023-10-30

|

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

codecov · 2023-10-31T04:17:49Z

Codecov Report

Merging #4311 (ece618d) into main (6e4ad7e) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4311      +/-   ##
==========================================
+ Coverage   75.28%   75.31%   +0.03%     
==========================================
  Files         223      223              
  Lines       12357    12357              
  Branches     3198     3198              
==========================================
+ Hits         9303     9307       +4     
+ Misses       3054     3050       -4

see 3 files with indirect coverage changes

packages/create-cloudflare/src/helpers/command.ts

dario-piotrowicz · 2023-10-31T10:13:39Z

packages/create-cloudflare/src/helpers/command.ts

+	// So to retain the ability to lock verisons we run it with `npx` and spoof
+	// the user agent so scaffolding tools treat the invocation like yarn
+	let cmd = `${npm === "yarn" ? "npx" : dlx} ${cli} ${argString}`;
+	const env = npm === "yarn" ? { npm_config_user_agent: "yarn" } : {};


why is this needed?

So that the scaffolding tools think that they were invoked via yarn create my-framework. This means that they'll use yarn when doing any npm installs, which in turn means that we don't need to resetPackageManager.

ah ok, so here is basically when we run npx instead of yarn and we add this user agent to trick the underlying CLIs to think that we're actually using yarn?

I assume the code comments were added after this discussion?

yusukebe · 2023-10-31T10:32:12Z

Thanks for handling the issue!

RamIdeas · 2023-10-31T23:40:27Z

packages/create-cloudflare/src/helpers/command.ts

@@ -254,18 +263,34 @@ export const npmInstall = async () => {
 	});
 };

+const needsReset = (ctx: PagesGeneratorContext) => {


I'm not sure needsReset is named adequately in a file called command.ts

If this is the prerequisite check before calling resetPackageManager, maybe name is packageManagerNeedsReset

Since it is local to this file and only used in one place I don't think this name is too problematic.

petebacondarwin · 2023-11-01T19:40:08Z

.changeset/dirty-bobcats-eat.md

+"create-cloudflare": patch
+---
+
+Fixes c3 usage with yarn


Can we have a bit more explanation here? This will appear in the changelog for the package. We should write from the point of view of someone trying to find out what has changed in a release.

packages/create-cloudflare/src/frameworks/qwik/index.ts

petebacondarwin · 2023-11-01T19:56:23Z

packages/create-cloudflare/src/helpers/command.ts

 // Resets the package manager context for a project by clearing out existing dependencies
 // and lock files then re-installing.
 export const resetPackageManager = async (ctx: PagesGeneratorContext) => {
 	const { npm } = detectPackageManager();

-	// Only do this when using pnpm or yarn
-	if (npm === "npm") return;
+	if (!needsReset(ctx)) return;


Surely we have a lint rule that stops if blocks not in curlies?
I know these are only single line ifs, and I'm being a bit pedantic, but this is a very common cause of errors.

I agree it's does feel a bit unsafe (although I understand the convenience)

I don't think we have the eslint rule set and actually we do this sort of thing in a bunch of different places (C3 not being the biggest offender):

I'm totally on board adding the rule in a followup PR if devprod's on board with it 🙂

petebacondarwin

A few minor nits

jculvey requested review from a team as code owners October 31, 2023 04:07

jculvey changed the title ~~C3 fix yarn create~~ C3: Fix usage with yarn Oct 31, 2023

dario-piotrowicz approved these changes Oct 31, 2023

View reviewed changes

RamIdeas approved these changes Oct 31, 2023

View reviewed changes

petebacondarwin reviewed Nov 1, 2023

View reviewed changes

packages/create-cloudflare/src/frameworks/qwik/index.ts Show resolved Hide resolved

petebacondarwin reviewed Nov 1, 2023

View reviewed changes

petebacondarwin approved these changes Nov 1, 2023

View reviewed changes

jculvey force-pushed the c3-fix-yarn-create branch from 1266aca to 0f8b002 Compare November 7, 2023 17:27

jculvey added 5 commits November 7, 2023 12:52

C3: Fix yarn issues

d617cc9

changeset

0403d4d

Add pnpm to unsupportedPms for gatsby

e854c88

Fix typo in comment

d539ad4

Respond to PR feedback

ece618d

jculvey force-pushed the c3-fix-yarn-create branch from 0f8b002 to ece618d Compare November 7, 2023 18:52

jculvey merged commit 35be459 into main Nov 7, 2023
24 of 27 checks passed

jculvey deleted the c3-fix-yarn-create branch November 7, 2023 22:29

workers-devprod mentioned this pull request Nov 7, 2023

Version Packages #4291

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C3: Fix usage with yarn #4311

C3: Fix usage with yarn #4311

jculvey commented Oct 31, 2023 •

edited

Loading

changeset-bot bot commented Oct 31, 2023 •

edited

Loading

github-actions bot commented Oct 31, 2023 •

edited

Loading

codecov bot commented Oct 31, 2023 •

edited

Loading

dario-piotrowicz Oct 31, 2023

jculvey Oct 31, 2023

dario-piotrowicz Oct 31, 2023

jculvey Oct 31, 2023

petebacondarwin Nov 1, 2023

yusukebe commented Oct 31, 2023

RamIdeas Oct 31, 2023

petebacondarwin Nov 1, 2023

petebacondarwin Nov 1, 2023

petebacondarwin Nov 1, 2023

dario-piotrowicz Nov 2, 2023

petebacondarwin left a comment

C3: Fix usage with yarn #4311

C3: Fix usage with yarn #4311

Conversation

jculvey commented Oct 31, 2023 • edited Loading

changeset-bot bot commented Oct 31, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Oct 31, 2023 • edited Loading

codecov bot commented Oct 31, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yusukebe commented Oct 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

jculvey commented Oct 31, 2023 •

edited

Loading

changeset-bot bot commented Oct 31, 2023 •

edited

Loading

github-actions bot commented Oct 31, 2023 •

edited

Loading

codecov bot commented Oct 31, 2023 •

edited

Loading