-
Notifications
You must be signed in to change notification settings - Fork 615
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
Simplify pages download config #5569
Conversation
🦋 Changeset detectedLatest commit: a7544a9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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/9797569837/npm-package-wrangler-5569 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5569/npm-package-wrangler-5569 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9797569837/npm-package-wrangler-5569 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9797569837/npm-package-create-cloudflare-5569 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9797569837/npm-package-cloudflare-kv-asset-handler-5569 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9797569837/npm-package-miniflare-5569 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9797569837/npm-package-cloudflare-pages-shared-5569 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9797569837/npm-package-cloudflare-vitest-pool-workers-5569 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
587d23c
to
d0a5c97
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.
Marking as request changes because I think it would be good to have a discussion and resolution of the behaviour before we land this, not because I am insisting that we change the behaviour.
packages/wrangler/src/__tests__/pages/pages-download-config.test.ts
Outdated
Show resolved
Hide resolved
packages/wrangler/src/__tests__/pages/pages-download-config.test.ts
Outdated
Show resolved
Hide resolved
packages/wrangler/src/__tests__/pages/pages-download-config.test.ts
Outdated
Show resolved
Hide resolved
packages/wrangler/src/__tests__/pages/pages-download-config.test.ts
Outdated
Show resolved
Hide resolved
@penalosa - can you follow up on this PR and address my comments? |
ee2e5eb
to
36761c3
Compare
production: RawEnvironment; | ||
} { | ||
const topLevel = { ...preview }; | ||
// Remove duplication for inheritable keys (https://developers.cloudflare.com/pages/functions/wrangler-configuration/#inheritable-keys) |
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 am a bit concerned about this logic (that find inheritable props to deduplicate and that handles the props that cannot be removed from an environment - e.g. limits
) is hidden inside this function.
What happens when someone adds a new inheritable field to the configuration in the future. How are they goingt to know that they must come here and update this logic?
Could we consider making this a bit more declarative? Perhaps having a list of such properties in the config/environment.ts
file where these aspects are defined and then this code would just generically use that list without having to hard code properties in 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.
We discussed this offline, and decided to stay roughly with this approach for now, since the exceptions aren't super generic
What this PR solves / how to test
Simplifies the config file downloaded by
wrangler pages download config
:Fixes DEVX-1245
Fixes DEVX-1245
Author has addressed the following