cloudflare / workers-sdk

⛅️ Home to Wrangler, the CLI for Cloudflare Workers®
https://developers.cloudflare.com/workers/
Apache License 2.0
2.41k stars 592 forks source link

chore: fail build on eslint warnings #6111

Closed threepointone closed 1 week ago

threepointone commented 1 week ago

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

changeset-bot[bot] commented 1 week ago

⚠️ No Changeset found

Latest commit: db58c8b01fc776bcb04ab70150270dd63444b74e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions[bot] commented 1 week ago

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 with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9664540137/npm-package-wrangler-6111 dev path/to/script.js
Additional artifacts: ```sh npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9664540137/npm-package-create-cloudflare-6111 --no-auto-update ``` ```sh npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9664540137/npm-package-cloudflare-kv-asset-handler-6111 ``` ```sh npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9664540137/npm-package-miniflare-6111 ``` ```sh npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9664540137/npm-package-cloudflare-pages-shared-6111 ``` ```sh 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](https://docs.github.com/en/organizations/managing-organization-settings/configuring-the-retention-period-for-github-actions-artifacts-and-logs-in-your-organization).

wrangler@3.62.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240620.0
workerd 1.20240620.1 1.20240620.1
workerd --version 1.20240620.1 2024-06-20

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

DaniFoldi commented 1 week ago

Hey Sunil :wave:,

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 error? That way local (IDE) and CI experience is consistent. Flat eslint configs (while a definite increase in complexity over the old system) are just js arrays/objects, so mapping warnings to errors should be relatively simple even with presets.

threepointone commented 1 week ago

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.