-
Notifications
You must be signed in to change notification settings - Fork 786
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
C3: add vite+react templates with tests #8013
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: fb1da7a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
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/13458024202/npm-package-wrangler-8013 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/8013/npm-package-wrangler-8013 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13458024202/npm-package-wrangler-8013 dev path/to/script.js Additional artifacts:cloudflare-workers-bindings-extension: wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13458024202/npm-package-cloudflare-workers-bindings-extension-8013 -O ./cloudflare-workers-bindings-extension.0.0.0-vfd6b10fe3.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-vfd6b10fe3.vsix create-cloudflare: npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13458024202/npm-package-create-cloudflare-8013 --no-auto-update @cloudflare/kv-asset-handler: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13458024202/npm-package-cloudflare-kv-asset-handler-8013 miniflare: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13458024202/npm-package-miniflare-8013 @cloudflare/pages-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13458024202/npm-package-cloudflare-pages-shared-8013 @cloudflare/unenv-preset: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13458024202/npm-package-cloudflare-unenv-preset-8013 @cloudflare/vite-plugin: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13458024202/npm-package-cloudflare-vite-plugin-8013 @cloudflare/vitest-pool-workers: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13458024202/npm-package-cloudflare-vitest-pool-workers-8013 @cloudflare/workers-editor-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13458024202/npm-package-cloudflare-workers-editor-shared-8013 @cloudflare/workers-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13458024202/npm-package-cloudflare-workers-shared-8013 @cloudflare/workflows-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13458024202/npm-package-cloudflare-workflows-shared-8013 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
6e2abcc
to
25eed83
Compare
7c911d7
to
fef0bbc
Compare
2967583
to
600cadf
Compare
@@ -107,6 +107,8 @@ function getFrameworkTests(opts: { | |||
input: [keys.enter], | |||
}, | |||
], | |||
unsupportedOSs: ["win32"], | |||
unsupportedPms: ["yarn"], |
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.
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.
Create-vite basically tries to parse the npm_config_user_agent
environment variable - expecting it to look like: yarn/1.22.22 node/20.0.0
etc - to grab the version of yarn.
Because we are running create-cloudflare via pnpx
and "faking" the package manager it doesn't work. Something (I think the bit of pnpx/npx that actually creates the child process) is stripping the version so npm_config_user_agent
ends up just looking like: yarn
(no version).
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.
Do you think there's any way to fix this?
@@ -25,7 +25,7 @@ export const detectPackageManager = () => { | |||
if (process.env.TEST_PM && process.env.TEST_PM_VERSION) { | |||
name = process.env.TEST_PM as PmName; | |||
version = process.env.TEST_PM_VERSION; | |||
process.env.npm_config_user_agent = name; | |||
process.env.npm_config_user_agent = `${name}/${version}`; |
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.
This was an attempt to get yarn
working with create-vite
but something appears to be stripping the version between us running pnpx
and create-vite
being run.
input: [keys.enter], | ||
}, | ||
], | ||
unsupportedOSs: ["win32"], |
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.
The current create-vite
template doesn't pass on Windows either, so this is not a regression. But at some point we should look into fixing that.
}, | ||
verifyPreview: { | ||
route: "/", | ||
previewArgs: ["--host=127.0.0.1"], |
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.
This is necessary because by default vite preview
is only listening on localhost
, which doesn't appear to include 127.0.0.1
(at least on some OSes).
@@ -742,6 +762,7 @@ const runCli = async ( | |||
NO_DEPLOY ? "--no-deploy" : "--deploy", | |||
"--no-open", | |||
"--no-git", | |||
"--no-auto-update", |
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.
This is probably not really needed but seemed like a safe thing to do to ensure that we are actually running the instance of C3 that we expected.
{ | ||
value: "react-swc-ts", | ||
lang: "ts", | ||
label: "TypeScript + SWC", |
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.
what does SWC stand for? and are we sure users will have this context?
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.
This is how it appears in the create-vite
CLI so I think it's OK.
Name from API is: {name} | ||
</button> | ||
<p> | ||
Edit <code>api/index.ts</code> to change the name |
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.
shouldn't this be <code>api/index.js</code>
?
@@ -0,0 +1,9 @@ | |||
{ | |||
"name": "<TBD>", | |||
"main": "api/index.js", |
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.
indentation here is odd 🙃 . no biggie just calling it out. prolly prettier will fix, if you haven't run it already
@@ -74,7 +74,8 @@ | |||
"@types/react-transition-group>@types/react": "^18", | |||
"@cloudflare/elements>@types/react": "^18", | |||
"capnpc-ts>typescript": "4.2.4", | |||
"@types/node": "$@types/node" | |||
"@types/node": "$@types/node", | |||
"vitest>vite": "^5.0.0" |
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.
what is this >
magic?
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.
It is standard pnpm magic, which means override only vite
version but only those that are dependencies of vitest
.
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.
looks like smth is broken when running this pre-release C3 version with the two js templates. ts templates are working just fine 🚀
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.
Few minor suggestions. Not approving yet because the JS templates fail with this error:
ERROR TypeError: Cannot read properties of undefined (reading 'value')
@@ -107,6 +107,8 @@ function getFrameworkTests(opts: { | |||
input: [keys.enter], | |||
}, | |||
], | |||
unsupportedOSs: ["win32"], | |||
unsupportedPms: ["yarn"], |
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.
Do you think there's any way to fix this?
* | ||
* This is rather than having a wrangler.toml in the e2e test's fixture folder, | ||
* This is rather than having a wrangler configuration file in the e2e test's fixture folder, | ||
* which overwrites any that comes from the framework's template. | ||
*/ | ||
const addTestVarsToWranglerToml = async (projectPath: string) => { |
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.
Maybe not for this PR, but the name of this variable and all the references to wrangler.toml
should be updated to be format agnostic.
{ | ||
value: "react-swc-ts", | ||
lang: "ts", | ||
label: "TypeScript + SWC", |
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.
This is how it appears in the create-vite
CLI so I think it's OK.
@@ -0,0 +1,54 @@ | |||
import { useState } from "react"; |
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.
I think if we're only updating particular files in the template then we need to match the formatting of the Vite template. The prettier config should be:
{
"semi": false,
"singleQuote": true,
}
tsconfig.references.push({ path: "./tsconfig.worker.json" }); | ||
} | ||
writeJSON("tsconfig.json", tsconfig); | ||
s.stop(`${brandColor(`updated`)} ${dim(`\`angular.json\``)}`); |
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.
s.stop(`${brandColor(`updated`)} ${dim(`\`angular.json\``)}`); | |
s.stop(`${brandColor(`updated`)} ${dim(`\`tsconfig.json\``)}`); |
await installPackages(["@cloudflare/vite-plugin"], { | ||
dev: true, | ||
startText: "Installing the Cloudflare Vite plugin", | ||
doneText: `${brandColor(`updated`)} ${dim("wrangler@latest")}`, |
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.
doneText: `${brandColor(`updated`)} ${dim("wrangler@latest")}`, | |
doneText: `${brandColor(`installed`)} ${dim("@cloudflare/vite-plugin")}`, |
transformPackageJson: async () => ({ | ||
scripts: { | ||
deploy: `${npm} run build && wrangler deploy`, | ||
preview: `${npm} run build && vite preview`, |
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.
Why deviate from the Vite template here?
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.
It seems like you would need the build before running the preview, right?
await runFrameworkGenerator(ctx, [ | ||
ctx.project.name, | ||
"--template", | ||
variant?.value, |
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.
Why is this possibly undefined
?
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.
turns out it is not
This makes it possible to preserve comments in JSON(C) files that contain them.
The create-vite app wants to process the npm_config_user_agent to get the version of yarn, but for some reason this gets stripped by the fact we are faking it and actually running pnpx.
82250e4
to
fb1da7a
Compare
Fixes #0000
Adds experimental C3 templates for a React SPA using the new Cloudflare Vite plugin rather than Wrangler for local development.