* 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.
This removes all the remaining references to the `build2` directory
except for the CI job that stores the artifacts. We'll keep the
`build2` artifact until downstream scripts are migrated to `build`.
Update all our local scripts to use `build` instead of `build2`.
There are still downstream scripts that depend on `build2`, though, so
we can't remove it yet.
Now that all the CI jobs have been migrated to the new build script,
we can start renaming the `build2` directory to `build`.
Since there are lots of scripts that reference `build2`, including
downstream scripts that live outside this repo, I'm going to keep
the `build2` directory around as a copy of `build`.
Then once all the references are updated, I will delete the copy.
This flag was broken due to a buggy race case in the ncp() command. The fix is amittedly a hack but improves on the existing behavior (of leaving the workspace in a broken state).
This sets up an initial shim implementation of useSyncExternalStore,
via the use-sync-external-store package. It's designed to mimic the
behavior of the built-in API, but is backwards compatible to any version
of React that supports hooks.
I have not yet implemented the built-in API, but once it exists, the
use-sync-external-store package will always prefer that one. Library
authors can depend on the shim and trust that their users get the
correct implementation.
See https://github.com/reactwg/react-18/discussions/86 for background
on the API.
The tests I've added here are designed to run against both the shim and
built-in implementation, using our variant test flag feature. Tests that
only apply to concurrent roots will live in a separate suite.
* Re-add old Fabric Offscreen impl behind flag
There's a chance that #21960 will affect layout in a way that we don't
expect, so I'm adding back the old implementation so we can toggle the
feature with a flag.
The flag should read from the ReactNativeFeatureFlags shim so that we
can change it at runtime. I'll do that separately.
* Import dynamic RN flags from external module
Internal feature flags that we wish to control with a GK can now be
imported from an external module, which I've called
"ReactNativeInternalFeatureFlags".
We'll need to add this module to the downstream repo.
We can't yet use this in our tests, because we don't have a test
configuration that runs against the React Native feature flags fork. We
should set up that up the same way we did for www.
Change format of @next and @experimental release versions from <number>-<sha> to <number>-<sha>-<date> to make them more human readable. This format still preserves the ability for us to easily map a version number to the changes it contains, while also being able to more easily know at a glance how recent a release is.
* Move internal version of act to shared module
No reason to have three different copies of this anymore.
I've left the the renderer-specific `act` entry points because legacy
mode tests need to also be wrapped in `batchedUpdates`. Next, I'll update
the tests to use `batchedUpdates` manually when needed.
* Migrates tests to use internal module directly
Instead of the `unstable_concurrentAct` exports. Now we can drop those
from the public builds.
I put it in the jest-react package since that's where we put our other
testing utilities (like `toFlushAndYield`). Not so much so it can be
consumed publicly (nobody uses that package except us), but so it works
with our build tests.
* Remove unused internal fields
These were used by the old act implementation. No longer needed.
* Use the server src files as entry points for the builds/tests
We need one top level entry point to target two builds so we can't have
the top level one be the entry point for the builds.
* Same thing but with the modern entry point
* Wire up DOM legacy build
* Hack to filter extra comments for testing purposes
* Use string concat in renderToString
I think this might be faster. We could probably use a combination of this
technique in the stream too to lower the overhead.
* Error if we can't complete the root synchronously
Maybe this should always error but in the async forms we can just delay
the stream until it resolves so it does have some useful semantics.
In the synchronous form it's never useful though. I'm mostly adding the
error because we're testing this behavior for renderToString specifically.
* Gate memory leak tests of internals
These tests don't translate as is to the new implementation and have been
ported to the Fizz tests separately.
* Enable Fizz legacy mode in stable
* Add wrapper around the ServerFormatConfig for legacy mode
This ensures that we can inject custom overrides without negatively
affecting the new implementation.
This adds another field for static mark up for example.
* Wrap pushTextInstance to avoid emitting comments for text in static markup
* Don't emit static mark up for completed suspense boundaries
Completed and client rendered boundaries are only marked for the client
to take over.
Pending boundaries are still supported in case you stream non-hydratable
mark up.
* Wire up generateStaticMarkup to static API entry points
* Mark as renderer for stable
This shouldn't affect the FB one ideally but it's done with the same build
so let's hope this works.
This does not mean that a release of 18.0 is imminent, only that the
main branch includes breaking changes.
Also updates the versioning scheme of the `@next` channel to include
the upcoming semver number, as well as the word "alpha" to indicate the
stability of the release.
- Before: 0.0.0-e0d9b28999
- After: 18.0.0-alpha-e0d9b28999
Now that we track package versions in source, `@latest` builds should
be fully reproducible for a given commit. We can prepare the packages in
CI and store them as artifacts, the same way we do for `@next` and
`@experimental`.
Eventually this can replace the interactive script that we currently
use to swap out the version numbers.
The other nice thing about this approach is that we can run tests in CI
to verify that the packages are releasable, instead of waiting until
right before publish.
I named the output directory `oss-stable-semver`, to distinguish from
the `@next` prereleases that are located at `oss-stable`. I don't love
this naming. I'd prefer to use the name of the corresponding npm dist
tag. I'll do that in a follow-up, though, since the `oss-stable` name is
referenced in a handful of places.
Current naming (after this PR):
- `oss-experimental` → `@experimental`
- `oss-stable` → `@next`
- `oss-stable-semver` → `@latest`
Proposed naming (not yet implemented, requires more work):
- `oss-experimental` → `@experimental`
- `oss-next` → `@next`
- `oss-latest` → `@latest`
The versioning scheme for `@next` releases does not include semver
information. Like `@experimental`, the versions are based only on the
hash, i.e. `0.0.0-<commit_sha>`. The reason we do this is to prevent
the use of a tilde (~) or caret (^) to match a range of
prerelease versions.
For `@experimental`, I think this rationale still makes sense — those
releases are very unstable, with frequent breaking changes. But `@next`
is not as volatile. It represents the next stable release. So, I think
we can afford to include an actual verison number at the beginning of
the string instead of `0.0.0`.
We can also add a label that indicates readiness of the upcoming
release, like "alpha", "beta", "rc", etc.
To prepare for this the new versioning scheme, I updated the build
script. However, **this PR does not enable the new versioning scheme
yet**. I left a TODO above the line that we'll change once we're ready.
We need to specify the expected next version numbers for each package,
somewhere. These aren't encoded anywhere today — we don't specify
version numbers until right before publishing to `@latest`, using an
interactive script: `prepare-release-from-npm`.
Instead, what we can do is track these version numbers in a module. I
added `ReactVersions.js` that acts as the single source of truth for
every package's version. The build script uses this module to build the
`@next` packages.
In the future, I want to start building the `@latest` packages the same
way we do `@next` and `@experimental`. (What we do now is download a
`@next` release from npm and swap out its version numbers.) Then we
could run automated tests in CI to confirm the packages are releasable,
instead of waiting to verify that right before publish.
* Clean up Scheduler forks
* Un-shadow variables
* Use timer globals directly, add a test for overrides
* Remove more window references
* Don't crash for undefined globals + tests
* Update lint config globals
* Fix test by using async act
* Add test fixture
* Delete test fixture
Uses the layout of the build artifact directory to infer the format
of a given file, and which lint rules to apply.
This has the effect of decoupling the lint build job from the existing
Rollup script, so that if we ever add additional post-processing, or
if we replace Rollup, it will still work.
But the immediate motivation is to replace the separate "stable" and
"experimental" lint-build jobs with a single combined job.
The event priority constants exports by the reconciler package are
meant to be used by the reconciler (host config) itself. So it doesn't
make sense to export them from a module that requires them.
To break the cycle, we can move them to a separate module and import
that. This looks like a "deep import" of an internal module, which we
try to avoid, but conceptually these are part of the public interface
of the reconciler module. So, no different than importing from the main
`react-reconciler`.
We do need to be careful about not mixing these types of imports with
implementation details. Those are the ones to really avoid.
An unintended benefit of the reconciler fork infra is that it makes
deep imports harder. Any module that we treat as "public", like this
one, needs to account for the `enableNewReconciler` flag and forward
to the correct implementation.
Fixes issue in the new build workflow where the experimental packages do
not include "experimental" in the version string. This was because the
previous approach relied on the RELEASE_CHANNEL environment variable,
which we are no longer setting in the outer CI job, since we use the
same job to build both channels. To solve, I moved the version
post-processing into the build script itself.
Only affects the new build workflow. Old workflow is unchanged.
Longer term, I would like to remove version numbers from the source
entirely, including the package.jsons. We should use a placeholder
instead; that's mostly how it already works, since the release script
swaps out the versions before we publish to stable.
The goal is to simplify our CI pipeline so that all configurations
are built and tested in a single workflow.
As a first step, this adds a new build script entry point that builds
both the experimental and stable release channels into a single
artifacts directory.
The script works by wrapping the existing build script (which only
builds a single release channel at a time), then post-processing the
results to match the desired filesystem layout. A future version of the
build script would output the files directly without post-processing.
Because many parts of our infra depend on the existing layout of the
build artifacts directory, I have left the old workflows untouched.
We can incremental migrate to the new layout, then delete the old
workflows after we've finished.
* Don't allocate the inner cache unnecessarily
We only need it when we're asking for text. I anticipate I'll want to avoid allocating it in other methods too when it's not strictly necessary.
* Add fs.access
* Add fs.lstat
* Add fs.stat
* Add fs.readdir
* Add fs.readlink
* Add fs.realpath
* Rename functions to disambiguate two caches
* [Flight] Add rudimentary FS binding
* Throw for unsupported
* Don't mess with hidden class
* Use absolute path as the key
* Warn on relative and non-normalized paths
* Move files
* Update paths
* Rename import variables
* Rename /server to /writer
This is mainly because "React Server Server" is weird so we need another
dimension.
* Use "react-server" convention to enforce that writer is only loaded in a server
* Basic scan of the file system to find Client modules
This does a rudimentary merge of the plugins. It still uses the global
scan and writes to file system.
Now the plugin accepts a search path or a list of referenced client files.
In prod, the best practice is to provide a list of files that are actually
referenced rather than including everything possibly reachable. Probably
in dev too since it's faster.
This is using the same convention as the upstream ContextModule - which
powers the require.context helpers.
* Add neo-async to dependencies
* Remove react/unstable_cache
We're probably going to make it available via the dispatcher. Let's remove this for now.
* Add readContext() to the dispatcher
On the server, it will be per-request.
On the client, there will be some way to shadow it.
For now, I provide it on the server, and throw on the client.
* Use readContext() from react-fetch
This makes it work on the server (but not on the client until we implement it there.)
Updated the test to use Server Components. Now it passes.
* Fixture: Add fetch from a Server Component
* readCache -> getCacheForType<T>
* Add React.unstable_getCacheForType
* Add a feature flag
* Fix Flow
* Add react-suspense-test-utils and port tests
* Remove extra Map lookup
* Unroll async/await because build system
* Add some error coverage and retry
* Add unstable_getCacheForType to Flight entry
* Rename "name"->"filepath" field on Webpack module references
This field name will get confused with the imported name or the module id.
* Switch back to transformSource instead of getSource
getSource would be more efficient in the cases where we don't need to read
the original file but we'll need to most of the time.
Even then, we can't return a JS file if we're trying to support non-JS
loader because it'll end up being transformed.
Similarly, we'll need to parse the file and we can't parse it before it's
transformed. So we need to chain with other loaders that know how.
* Add acorn dependency
This should be the version used by Webpack since we have a dependency on
Webpack anyway.
* Parse exported names of ESM modules
We need to statically resolve the names that a client component will
export so that we can export a module reference for each of the names.
For export * from, this gets tricky because we need to also load the
source of the next file to parse that. We don't know exactly how the
client is built so we guess it's somewhat default.
* Handle imported names one level deep in CommonJS using a Proxy
We use a proxy to see what property the server access and that will tell
us which property we'll want to import on the client.
* Add export name to module reference and Webpack map
To support named exports each name needs to be encoded as a separate
reference. It's possible with module splitting that different exports end
up in different chunks.
It's also possible that the export is renamed as part of minification.
So the map also includes a map from the original to the bundled name.
* Special case plain CJS requires and conditional imports using __esModule
This models if the server tries to import .default or a plain require.
We should replicate the same thing on the client when we load that
module reference.
* Dedupe acorn-related deps
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
This convention ensures that you can declare that you intend for a file
to only be used on the server (even if it technically might resolve
on the client).
This configures the Flight fixture to use the "react-server" environment.
This allows the package.json exports field to specify a different resolution
in this environment.
I use this in the "react" package to resolve to a new bundle that excludes
the Hooks that aren't relevant in this environment like useState and useEffect.
This allows us to error early if these names are imported. If we actually
published ESM, it would we a static error. Now it's a runtime error.
You can test this by importing useState in Container.js which is used
by the client and server.
* Add Node ESM loader build
This adds a loader build as a first-class export. This will grow in
complexity so it deserves its own module.
* Add Node CommonJS regiter build
This adds a build as a first-class export for legacy CommonJS registration
in Node.js. This will grow in complexity so it deserves its own module.
* Simplify fixture a bit to easier show usage with or without esm
* Bump es version
We leave async function in here which are newer than ES2015.
This allows exporting ESM modules for the Webpack plugin. This is necessary
for making a resolver plugin. We could probably make the whole plugin
use ESM instead of CJS ES2015.
Justin Grant
Andrew Clark
salazarm
Brian Vaughn
Sebastian Markbåge
Samuel Susla
Dan Abramov
Ricky
Henry Q. Dineen
Hector Rincon
Sebastian Silbermann