-
Notifications
You must be signed in to change notification settings - Fork 758
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
Warn if unexpected key is in the toml #5689
Conversation
🦋 Changeset detectedLatest commit: da82270 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 |
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.
Thanks for improving the experience for Wrangler users. This looks like a great improvement. A couple of suggestions if you have time...
fixtures/messy-config/package.json
Outdated
"deploy": "wrangler deploy", | ||
"dev": "wrangler dev", | ||
"start": "wrangler dev", | ||
"test": "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.
Thanks for creating this fixture. As it stands I don't think this is running any tests in the CI. We could do with a vitest.config.js and then an actual vitest test that runs Wrangler to trigger the config warnings.
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'm not sure a full e2e test is warranted for this change. I realise I may have misunderstood the intent behind the fixtures when creating this. I treated it as more of an examples folder where I could put some code.
If you think it's best I'm happy to add one but looking through the existing tests in the repo a unit test here checking that the output contains the warnings would give me reasonable confidence in this change.
Thoughts?
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/8940208957/npm-package-wrangler-5689 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5689/npm-package-wrangler-5689 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8940208957/npm-package-wrangler-5689 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8940208957/npm-package-create-cloudflare-5689 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8940208957/npm-package-cloudflare-kv-asset-handler-5689 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8940208957/npm-package-miniflare-5689 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8940208957/npm-package-cloudflare-pages-shared-5689 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8940208957/npm-package-cloudflare-vitest-pool-workers-5689 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
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.
Cool. Thanks for updating. Try running:
pnpm run fix
to sort out the formatting issues
The other failures are most likely flakes
Thanks @petebacondarwin I have ran those formatting checks. Hopefully the CI flakes in my favour this time 😃 |
The other thing that is worth flagging is I used the json schema and a script to generate the lists of optional fields so I didn't have to write them by hand. There is a chance that a missing field in my configuration could lead to unexpected warnings in customers wrangler tomls. I could think of a good way to verify this but here is the script I used. const schema = require('./config-schema.json')
const environment = schema.definitions.Environment.properties
function generateCode(properties) {
return `validateAdditionalProperties(diagnostics, field, Object.keys(value), [${
properties.reduce((sum, el) => `${sum}\n\t"${el}",`, '')
}
]);
`
}
for(const service of schema.definitions.Environment.required) {
console.log(service, '\n')
const serviceConfig = environment[service].properties || environment[service].items
if(serviceConfig && !serviceConfig.additionalProperties && serviceConfig.properties) {
// Generate code
console.log(generateCode(Object.keys(serviceConfig.properties)))
} else if(serviceConfig) {
for(const configuration of Object.values(serviceConfig)) {
if(configuration.items?.additionalProperties === false) {
// Generate code
console.log(generateCode(Object.keys(configuration.items.properties)))
}
}
}
} |
What this PR solves / how to test
Fixes #5681
Author has addressed the following