Skip to content

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