Skip to content

Remove duplicate error code #22513

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

Merged
merged 1 commit into from
Oct 6, 2021
Merged

Conversation

sebmarkbage
Copy link
Collaborator

The codemod seem to have found this error message twice.

There's kind of a flaw in the extract-errors script in that it uses map from message to ID which doesn't allow multiple IDs with the same name. That could happen if we end up changing names of something in the future.

Currently this means that running extract-errors will delete a row.

We should never delete a code that has been used. Even an unreleased one can show up in FB error logs even in the future.

However, since this is so recent, this shouldn't have leaked much yet so maybe it's fine to just delete this duplicate.

@sebmarkbage sebmarkbage requested a review from acdlite October 6, 2021 02:06
@facebook-github-bot facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Oct 6, 2021
@sizebot
Copy link

sizebot commented Oct 6, 2021

Comparing: cadf94d...242608b

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.min.js = 130.05 kB 130.05 kB = 41.34 kB 41.34 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 132.88 kB 132.88 kB = 42.31 kB 42.31 kB
facebook-www/ReactDOM-prod.classic.js = 413.66 kB 413.66 kB = 76.44 kB 76.44 kB
facebook-www/ReactDOM-prod.modern.js = 402.24 kB 402.24 kB = 74.71 kB 74.71 kB
facebook-www/ReactDOMForked-prod.classic.js = 413.66 kB 413.66 kB = 76.45 kB 76.45 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 242608b

@acdlite
Copy link
Collaborator

acdlite commented Oct 6, 2021

Oops, that happened when I was resolving conflicts. So a manual error as opposed to an automated one.

extract-errors doesn't event work right now because it relied on invariant... I actually forgot that exists because I always update it manually.

What I did do is make it so that ESLint prints out the message format if it detects one is missing or incorrect. So I think the common workflow will be to copy from there. I briefly looked into writing an autofix rule for this (which would replace the need for extract-errors but AFAICT ESLint's autofixes can't modify the file system, only the AST.

So I think what I'd do is update extract-errors to run the ESLint rule, collect the missing messages (which would account for duplicates, since it the rule checks whether it already exists in the map), and then append to the end of the file.

@sebmarkbage sebmarkbage merged commit 6485ef7 into facebook:main Oct 6, 2021
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
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