-
Notifications
You must be signed in to change notification settings - Fork 712
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
chore: update to turbo v2 #6083
Conversation
|
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/9583693363/npm-package-wrangler-6083 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6083/npm-package-wrangler-6083 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9583693363/npm-package-wrangler-6083 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9583693363/npm-package-create-cloudflare-6083 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9583693363/npm-package-cloudflare-kv-asset-handler-6083 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9583693363/npm-package-miniflare-6083 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9583693363/npm-package-cloudflare-pages-shared-6083 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9583693363/npm-package-cloudflare-vitest-pool-workers-6083 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
7ed217f
to
cb23bfc
Compare
d626d6b
to
bc584bb
Compare
1a5340f
to
ff2e3bb
Compare
"build": { | ||
"env": ["TEST_PM", "TEST_PM_VERSION", "npm_config_user_agent", "CI"], | ||
"env": [ | ||
"TMPDIR", |
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.
turbo needs you to whitelist any nv vars that you'd use. We have to add these new ones so that os.tmpdir() works correctly (which depend on them)
"build": { | ||
"inputs": ["src/**", "scripts/**", "types/**", "*.mjs", "*.js", "*.json"], | ||
"outputs": ["dist/**", "bootstrap.js", "worker-metafiles/**"], | ||
"env": ["CI_OS"] | ||
"env": ["CI_OS", "NODE_EXTRA_CA_CERTS"] |
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.
without this, the miniflare test Miniflare: HTTPS fetches using browser CA certificates
will fail
ff2e3bb
to
48b02b4
Compare
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.
A couple of concerns that need resolving (either by fixing or telling me I'm wrong).
packages/wrangler/turbo.json
Outdated
"NODE_EXTRA_CA_CERTS", | ||
"CLOUDFLARE_ACCOUNT_ID", | ||
"SOURCEMAPS", | ||
"NODE_ENV", | ||
"SPARROW_SOURCE_KEY", | ||
"ALGOLIA_APP_ID", | ||
"ALGOLIA_PUBLIC_KEY", | ||
"NO_SCRIPT_SIZE_WARNING", | ||
"CLOUDFLARE_API_TOKEN", | ||
"CLOUDFLARE_ACCOUNT_ID", | ||
"WRANGLER_AUTH_DOMAIN", | ||
"PATH", | ||
"WRANGLER_LOG", | ||
"EXPERIMENTAL_MIDDLEWARE", | ||
"FORMAT_WRANGLER_ERRORS", | ||
"CF_PAGES", | ||
"CI", | ||
"CF_PAGES_UPLOAD_JWT", | ||
"NO_SCRIPT_SIZE_WARNING", | ||
"EXPERIMENTAL_MIDDLEWARE", | ||
"NO_D1_WARNING", | ||
"NO_HYPERDRIVE_WARNING", | ||
"WRANGLER", | ||
"WRANGLER_IMPORT", | ||
"CUSTOM_BUILD_VAR", | ||
"PWD", | ||
"LC_ALL", | ||
"WRANGLER_SEND_METRICS", | ||
"https_proxy", | ||
"HTTPS_PROXY", | ||
"http_proxy", | ||
"HTTP_PROXY", | ||
"CI_OS", | ||
"SENTRY_DSN", | ||
"SYSTEMROOT", |
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.
All these feel unnecessary. If they are only in the source files then they are already covered by the build
task. Here we should only be declaring the env vars that are referenced in the input files (i.e. "e2e/**"
).
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.
Similarly for the test:ci above.
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.
If they're read dynamically, then they have to be whitelisted. I considered being more precise, but that would make this more confusing for the future. Instead, I duplicate them across tasks because it's easier to make changes in the future too (where the instruction will be "hey just add it to both"
}, | ||
"test:ci": { | ||
"inputs": ["test/**"], | ||
"dependsOn": ["build"] | ||
"dependsOn": ["build"], | ||
"env": ["CI_OS", "NODE_EXTRA_CA_CERTS"] |
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.
Necessary? This should be covered by the build task?
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 test runner reads it dynamically, and it has to be whitelisted here for it to be added to the environment
packages/cli/turbo.json
Outdated
"build": { | ||
"outputs": ["dist/**"] | ||
} |
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.
Does overriding this here also override the dependsOn
field at the top level?
Do we need to include that?
I see that this package depends upon the eslint and tsconfig packages, so we need those deps marked in turbo so that we don't fail to rebuild this package when those change.
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 thin kI can just remove this one, I can't recollect why I put it here at all.
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 guess it improves the upstream tasks that rely upon this task?
48b02b4
to
b203da0
Compare
b203da0
to
10686b5
Compare
Apologies, completely missed this PR, and did this separately in #7076 |
Updates to turbo v2.
Author has addressed the following