-
Notifications
You must be signed in to change notification settings - Fork 790
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: fail build on eslint warnings #6111
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/9664540137/npm-package-wrangler-6111 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6111/npm-package-wrangler-6111 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9664540137/npm-package-wrangler-6111 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9664540137/npm-package-create-cloudflare-6111 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9664540137/npm-package-cloudflare-kv-asset-handler-6111 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9664540137/npm-package-miniflare-6111 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9664540137/npm-package-cloudflare-pages-shared-6111 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9664540137/npm-package-cloudflare-vitest-pool-workers-6111 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
a5e1742
to
23b79d4
Compare
Hey Sunil 👋, I'm not sure if changing the scripts here is the best approach for contributors. In IDEs warnings and errors have separate colours for highlighting, and most people are probably used to red being errors, and yellow being warnings. Obviously, I'd avoid introducing new warnings whenever possible, however if the goal is for a rule (or all rules) to prevent a change from being merged, wouldn't the best way to achieve that be setting the severity of that rule (or all rules) to |
Warnings signal that, with good judgement, they can be disabled. While an error signals that it's usually a mistake and may cause problems down the line. This doesn't change anything about the dx, but we don't want to land code that hasn't had a call taken. For example the linked code shouldn't have landed and should have broken the build, even though it wasn't super serious. I wouldn't want that standing out as an error while developing, feels too aggressive. |
23b79d4
to
7226bba
Compare
ed5ab8c
to
480111e
Compare
480111e
to
d38e9ff
Compare
modify all the eslint commands to fail if any warnings are present
d38e9ff
to
db58c8b
Compare
Modify all the eslint commands to fail if any warnings are present.
This should fail, and pass once https://github.com/cloudflare/workers-sdk/pull/6001/files#r1648173873 is resolved