Skip to content

Conversation

@poteto
Copy link
Member

@poteto poteto commented Sep 5, 2025

Mechanical PR to migrate existing invariants to use the new CompilerDiagnostic infra @josephsavona added. Will tackle the others at a later time.


Stack created with Sapling. Best reviewed with ReviewStack.

Copy link
Member

@josephsavona josephsavona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only qualm about this is that it makes it a (too?) easy to skip the effort of supplying a proper message on error details. An alternative would be to add an error variant that's just meant for invariants, but that's probably not worth the hassle.

@poteto
Copy link
Member Author

poteto commented Sep 5, 2025

My only qualm about this is that it makes it a (too?) easy to skip the effort of supplying a proper message on error details. An alternative would be to add an error variant that's just meant for invariants, but that's probably not worth the hassle.

Hmm interesting, my thought was actually the opposite. I was thinking that having the message property be required (even if nullable) encourages adding a proper message at the time the invariant is added. And now it's a bit easier as well to go back and add more details when we go through the code again. I added a few as well in this stack while I was doing this

@react-sizebot
Copy link

react-sizebot commented Sep 5, 2025

Comparing: c4e2508...df05a6b

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.68 kB 6.68 kB = 1.83 kB 1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 530.65 kB 530.65 kB = 93.49 kB 93.49 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 658.12 kB 658.12 kB = 115.77 kB 115.77 kB
facebook-www/ReactDOM-prod.classic.js = 682.25 kB 682.25 kB = 119.80 kB 119.80 kB
facebook-www/ReactDOM-prod.modern.js = 672.68 kB 672.68 kB = 118.11 kB 118.11 kB
oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.js +2.11% 2,055.87 kB 2,099.24 kB +0.80% 298.76 kB 301.15 kB
oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.js +2.11% 2,055.87 kB 2,099.24 kB +0.80% 298.76 kB 301.15 kB
oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.js +2.11% 2,056.05 kB 2,099.43 kB +0.80% 298.77 kB 301.17 kB
oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js +2.11% 2,060.34 kB 2,103.72 kB +0.80% 299.76 kB 302.16 kB
oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js +2.11% 2,060.34 kB 2,103.72 kB +0.80% 299.76 kB 302.16 kB
oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js +2.11% 2,060.52 kB 2,103.90 kB +0.80% 299.78 kB 302.18 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.js +2.11% 2,055.87 kB 2,099.24 kB +0.80% 298.76 kB 301.15 kB
oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.js +2.11% 2,055.87 kB 2,099.24 kB +0.80% 298.76 kB 301.15 kB
oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.js +2.11% 2,056.05 kB 2,099.43 kB +0.80% 298.77 kB 301.17 kB
oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js +2.11% 2,060.34 kB 2,103.72 kB +0.80% 299.76 kB 302.16 kB
oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js +2.11% 2,060.34 kB 2,103.72 kB +0.80% 299.76 kB 302.16 kB
oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js +2.11% 2,060.52 kB 2,103.90 kB +0.80% 299.78 kB 302.18 kB

Generated by 🚫 dangerJS against df05a6b

poteto added a commit that referenced this pull request Sep 6, 2025
With #34176 we now have granular lint rules created for each compiler
ErrorCategory. However, we had remnants of our old error severities
still in use which makes reporting errors quite clunky. Previously you
would need to specify both a category and severity which often ended up
being the same.

This PR moves severity definition into our rules which are generated
from our categories. For now I decided to defer "upgrading" categories
from a simple string to a sum type since we are only using severities to
map errors to eslint severity.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34401).
* #34409
* #34404
* #34403
* #34402
* __->__ #34401
poteto added a commit that referenced this pull request Sep 6, 2025
Now that we have a new CompilerDiagnostic type (which the CompilerError
aggregate can hold), the old CompilerErrorDetail type can be marked as
deprecated. Eventually we should migrate everything to the new
CompilerDiagnostic type.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34402).
* #34409
* #34404
* #34403
* __->__ #34402
* #34401
…infra

Mechanical PR to migrate existing invariants to use the new CompilerDiagnostic infra @josephsavona added. Will tackle the others at a later time.
github-actions bot pushed a commit that referenced this pull request Sep 6, 2025
With #34176 we now have granular lint rules created for each compiler
ErrorCategory. However, we had remnants of our old error severities
still in use which makes reporting errors quite clunky. Previously you
would need to specify both a category and severity which often ended up
being the same.

This PR moves severity definition into our rules which are generated
from our categories. For now I decided to defer "upgrading" categories
from a simple string to a sum type since we are only using severities to
map errors to eslint severity.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34401).
* #34409
* #34404
* #34403
* #34402
* __->__ #34401

DiffTrain build for [60d9b97](60d9b97)
github-actions bot pushed a commit that referenced this pull request Sep 6, 2025
With #34176 we now have granular lint rules created for each compiler
ErrorCategory. However, we had remnants of our old error severities
still in use which makes reporting errors quite clunky. Previously you
would need to specify both a category and severity which often ended up
being the same.

This PR moves severity definition into our rules which are generated
from our categories. For now I decided to defer "upgrading" categories
from a simple string to a sum type since we are only using severities to
map errors to eslint severity.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34401).
* #34409
* #34404
* #34403
* #34402
* __->__ #34401

DiffTrain build for [60d9b97](60d9b97)
@poteto poteto merged commit 474f258 into main Sep 6, 2025
21 checks passed
@poteto poteto deleted the pr34403 branch September 6, 2025 16:58
github-actions bot pushed a commit that referenced this pull request Sep 6, 2025
…infra (#34403)

Mechanical PR to migrate existing invariants to use the new
CompilerDiagnostic infra @josephsavona added. Will tackle the others at
a later time.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34403).
* #34409
* #34404
* __->__ #34403

DiffTrain build for [474f258](474f258)
github-actions bot pushed a commit that referenced this pull request Sep 6, 2025
…infra (#34403)

Mechanical PR to migrate existing invariants to use the new
CompilerDiagnostic infra @josephsavona added. Will tackle the others at
a later time.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34403).
* #34409
* #34404
* __->__ #34403

DiffTrain build for [474f258](474f258)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed React Core Team Opened by a member of the React Core Team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants