## Summary
Fix typo in dangerfile.js which results in an unreachable code path
which ought to be hit when there is no matching base artifact during
DangerCI automated code review.
See:
221f3002ca/dangerfile.js (L73)
Compare:
221f3002ca/dangerfile.js (L171)
And the case which should hit this code path:
221f3002ca/dangerfile.js (L160)
Given the above context, the condition `Number === Infinity` is clearly
meant to be `decimal === Infinity`, which it will be if the `catch`
statement triggers when there is no matching base artifact. Without this
fix, the primitive value `Infinity` is passed to
`percentFormatter.format(decimal)`, resulting in the string `'+∞%'`.
With this fix, the resulting string will be the intended `'New file'`.
## [Resolves issue
32278](https://github.com/facebook/react/issues/32278)
When a React PR is opened CI will report large size changes. But for
critical packages like react-dom it reports always. In React 19 we moved
the build for react-dom the client reconciler from react-dom to
react-dom/client
This change adds react-dom-client artifacts for stable and oss channels
since that is originally what was being tracked. But since
react-dom/client always imports react-dom I left the original react-dom
packages as critical as well. They are small but it would be good to
keep an eye on them
This disables symbol renaming in production builds. The original
variable and function names are preserved. All other forms of
compression applied by Closure (dead code elimination, inlining, etc)
are unchanged — the final program is identical to what we were producing
before, just in a more readable form.
The motivation is to make it easier to debug React issues that only
occur in production — the same reason we decided to start shipping
sourcemaps in #28827 and #28827.
However, because most apps run their own minification step on their npm
dependencies, it's not necessary for us to minify the symbols before
publishing — it'll be handled the app, if desired.
This is the same strategy Meta has used to ship React for years. The
React build itself has unminified symbols, but they get minified as part
of Meta's regular build pipeline.
Even if an app does not minify their npm dependencies, gzip covers most
of the cost of symbol renaming anyway.
This saves us from having to ship sourcemaps, which means even apps that
don't have sourcemaps configured will be able to debug the React build
as easily as they would any other npm dependency.
The old version of prettier we were using didn't support the Flow syntax
to access properties in a type using `SomeType['prop']`. This updates
`prettier` and `rollup-plugin-prettier` to the latest versions.
I added the prettier config `arrowParens: "avoid"` to reduce the diff
size as the default has changed in Prettier 2.0. The largest amount of
changes comes from function expressions now having a space. This doesn't
have an option to preserve the old behavior, so we have to update this.
* Facebook -> Meta in copyright
rg --files | xargs sed -i 's#Copyright (c) Facebook, Inc. and its affiliates.#Copyright (c) Meta Platforms, Inc. and affiliates.#g'
* Manual tweaks
* Revise ESLint rules for string coercion
Currently, react uses `'' + value` to coerce mixed values to strings.
This code will throw for Temporal objects or symbols.
To make string-coercion safer and to improve user-facing error messages,
This commit adds a new ESLint rule called `safe-string-coercion`.
This rule has two modes: a production mode and a non-production mode.
* If the `isProductionUserAppCode` option is true, then `'' + value`
coercions are allowed (because they're faster, although they may
throw) and `String(value)` coercions are disallowed. Exception:
when building error messages or running DEV-only code in prod
files, `String()` should be used because it won't throw.
* If the `isProductionUserAppCode` option is false, then `'' + value`
coercions are disallowed (because they may throw, and in non-prod
code it's not worth the risk) and `String(value)` are allowed.
Production mode is used for all files which will be bundled with
developers' userland apps. Non-prod mode is used for all other React
code: tests, DEV blocks, devtools extension, etc.
In production mode, in addiiton to flagging `String(value)` calls,
the rule will also flag `'' + value` or `value + ''` coercions that may
throw. The rule is smart enough to silence itself in the following
"will never throw" cases:
* When the coercion is wrapped in a `typeof` test that restricts to safe
(non-symbol, non-object) types. Example:
if (typeof value === 'string' || typeof value === 'number') {
thisWontReport('' + value);
}
* When what's being coerced is a unary function result, because unary
functions never return an object or a symbol.
* When the coerced value is a commonly-used numeric identifier:
`i`, `idx`, or `lineNumber`.
* When the statement immeidately before the coercion is a DEV-only
call to a function from shared/CheckStringCoercion.js. This call is a
no-op in production, but in DEV it will show a console error
explaining the problem, then will throw right after a long explanatory
code comment so that debugger users will have an idea what's going on.
The check function call must be in the following format:
if (__DEV__) {
checkXxxxxStringCoercion(value);
};
Manually disabling the rule is usually not necessary because almost all
prod use of the `'' + value` pattern falls into one of the categories
above. But in the rare cases where the rule isn't smart enough to detect
safe usage (e.g. when a coercion is inside a nested ternary operator),
manually disabling the rule will be needed.
The rule should also be manually disabled in prod error handling code
where `String(value)` should be used for coercions, because it'd be
bad to throw while building an error message or stack trace!
The prod and non-prod modes have differentiated error messages to
explain how to do a proper coercion in that mode.
If a production check call is needed but is missing or incorrect
(e.g. not in a DEV block or not immediately before the coercion), then
a context-sensitive error message will be reported so that developers
can figure out what's wrong and how to fix the problem.
Because string coercions are now handled by the `safe-string-coercion`
rule, the `no-primitive-constructor` rule no longer flags `String()`
usage. It still flags `new String(value)` because that usage is almost
always a bug.
* Add DEV-only string coercion check functions
This commit adds DEV-only functions to check whether coercing
values to strings using the `'' + value` pattern will throw. If it will
throw, these functions will:
1. Display a console error with a friendly error message describing
the problem and the developer can fix it.
2. Perform the coercion, which will throw. Right before the line where
the throwing happens, there's a long code comment that will help
debugger users (or others looking at the exception call stack) figure
out what happened and how to fix the problem.
One of these check functions should be called before all string coercion
of user-provided values, except when the the coercion is guaranteed not
to throw, e.g.
* if inside a typeof check like `if (typeof value === 'string')`
* if coercing the result of a unary function like `+value` or `value++`
* if coercing a variable named in a whitelist of numeric identifiers:
`i`, `idx`, or `lineNumber`.
The new `safe-string-coercion` internal ESLint rule enforces that
these check functions are called when they are required.
Only use these check functions in production code that will be bundled
with user apps. For non-prod code (and for production error-handling
code), use `String(value)` instead which may be a little slower but will
never throw.
* Add failing tests for string coercion
Added failing tests to verify:
* That input, select, and textarea elements with value and defaultValue
set to Temporal-like objects which will throw when coerced to string
using the `'' + value` pattern.
* That text elements will throw for Temporal-like objects
* That dangerouslySetInnerHTML will *not* throw for Temporal-like
objects because this value is not cast to a string before passing to
the DOM.
* That keys that are Temporal-like objects will throw
All tests above validate the friendly error messages thrown.
* Use `String(value)` for coercion in non-prod files
This commit switches non-production code from `'' + value` (which
throws for Temporal objects and symbols) to instead use `String(value)`
which won't throw for these or other future plus-phobic types.
"Non-produciton code" includes anything not bundled into user apps:
* Tests and test utilities. Note that I didn't change legacy React
test fixtures because I assumed it was good for those files to
act just like old React, including coercion behavior.
* Build scripts
* Dev tools package - In addition to switching to `String`, I also
removed special-case code for coercing symbols which is now
unnecessary.
* Add DEV-only string coercion checks to prod files
This commit adds DEV-only function calls to to check if string coercion
using `'' + value` will throw, which it will if the value is a Temporal
object or a symbol because those types can't be added with `+`.
If it will throw, then in DEV these checks will show a console error
to help the user undertsand what went wrong and how to fix the
problem. After emitting the console error, the check functions will
retry the coercion which will throw with a call stack that's easy (or
at least easier!) to troubleshoot because the exception happens right
after a long comment explaining the issue. So whether the user is in
a debugger, looking at the browser console, or viewing the in-browser
DEV call stack, it should be easy to understand and fix the problem.
In most cases, the safe-string-coercion ESLint rule is smart enough to
detect when a coercion is safe. But in rare cases (e.g. when a coercion
is inside a ternary) this rule will have to be manually disabled.
This commit also switches error-handling code to use `String(value)`
for coercion, because it's bad to crash when you're trying to build
an error message or a call stack! Because `String()` is usually
disallowed by the `safe-string-coercion` ESLint rule in production
code, the rule must be disabled when `String()` is used.
Because we have access to the artifacts in CI, we can read bundle sizes
directly from the filesystem, instead of the JSON files emitted by our
custom Rollup plugin.
This gives us some flexibility if we ever have artifacts that aren't
generated by Rollup, or if we rewrite our build script.
Personally, I also prefer to see the whole file path, instead of just
the name, because some of our names are repeated.
My immediate motivation, though, is because it gives us a way to merge
the separate "experimental" and "stable" size results. Instead
everything is reported in a single table and disambiguated by path.
I also added a section at the top that always displays the size impact
to certain critical bundles — right now, that's the React DOM production
bundles for each release channel. This section will also include any
size changes larger than 2%.
Below that is a section that is collapsed by default and includes all
size changes larger than 0.2%.
* build-combined: Fix bundle sizes path
* Output COMMIT_SHA in build directory
Alternative to parsing an arbitrary package's version number, or
its `build-info.json`.
* Remove CircleCI environment variable requirement
I think this only reason we needed this was to support passing any
job id to `--build`, instead of requiring the `process_artifacts` job.
And to do that you needed to use the workflows API endpoint, which
requires an API token.
But now that the `--commit` argument exists and automatically finds the
correct job id, we can just use that.
* Add CI job that gets base artifacts
Uses download-experimental script and places the base artifacts into
a top-level folder.
* Migrate sizebot to combined workflow
Replaces the two separate sizebot jobs (one for each channel, stable and
experimental) with a single combined job that outputs size information
for all bundles in a single GitHub comment.
I didn't attempt to simplify the output at all, but we should. I think
what I would do is remove our custom Rollup sizes plugin, and read the
sizes from the filesystem instead. We would lose some information about
the build configuration used to generate each artifact, but that can be
inferred from the filepath. For example, the filepath
`fb-www/ReactDOM-dev.classic.js` already tells us everything we need to
know about the artifact. Leaving this for a follow up.
* Move GitHub status check inside retry loop
The download script will poll the CircleCI endpoint until the build job
is complete; it should also poll the GitHub status endpoint if the
build job hasn't been spawned yet.
* Only run get_base_build on main branch
Sometimes the status of the `build` job is not in the first page of
the `/statuses` endpoint. The combined `/status` endpoint consolidates
the entries, though, so it always appears there.
The sizebot scrapes the GitHub `/statuses` endpoint to get the lastest
CircleCI build number for master, in order to fetch the bundle size
info for that build, which are stored as build artifacts. (There's gotta
be a better way to do this, but that's what we have for now.) This
updates the script to match the name of the updated CircleCI job that
generates the bundle sizes.
* Don't download bundle stats from master on CI
This was temporarily necessary in the past because we didn't have the logic that downloads actual *merge base* stats.
We do have that now as part of the Danger script. So we can remove this.
* Use absolute threshold for whether to show a change
* Download master stats, but only for other master builds
* Rewrite sizes
* [Danger] Use the PR's mergebase for a branch in the dangerfile instead of
the root commit's parent.
* [Danger] Get the full history to find the merge base
* Adds danger_js with an initial rule for warning about large PRs
Signed-off-by: Anandaroop Roy <roop@artsymail.com>
* [WIP] Get the before and after for the build results
* [Dev] More work on the Dangerfile
* [Danger] Split the reports into sections based on their package
* Remove the --extract-errors on the circle build
* [Danger] Improve the lookup for previous -> current build to also include the environment
* Fix rebase
