* Support throwing null
In JavaScript, you can throw values of any type, not just errors. That
includes null. We currently rely on null checks to determine if a user-
provided function has thrown. This refactors our error handling code to
keep track of an explicit boolean flag instead.
* Add DOM fixture test case for break on exception behavior
* preventDefault error events during feature test
We call invokeGuardedCallbackDev at startup as part of a feature test.
But we don't want those errors to log to the console.
* Add throwing null test case
* Use ReactFeatureFlags instead of ReactDOMFeatureFlags
React ART uses this, too.
* Non-errors in error logger
If a non-error is thrown, we'll coerce the value to a string and use
that as the message.
* RFC Add warning for rendering into container that was updated manually
RFC because we still need to tidy this up and verify that all tests
pass.
**what is the change?:**
We want to warn when users render into a container which was manually
emptied or updated outside of React. This can lead to the cryptic error
about not being able to remove a node, or just lead to silent failures
of render. This warning should make things more clear.
Note that this covers the case where the contents of the root container
are manually updated, but does not cover the case where something was
manually updated deeper in the tree.
**why make this change?:**
To maintain parity and increase clarity before releasing v16.0 beta.
**test plan:**
`yarn test`
**issue:**
https://github.com/facebook/react/issues/8854
last item under the '16 beta' checklist.
* Add test and tweak check for rendering into manually updated container
STILL TODO: figure out how to skip this warning when the component
renders to a portal.
Unfortunately 'ReactPortal.isPortal(children)' returns false, even in
the failing test where we are rendering to a portal.
**what is the change?:**
- added a test for the case where we call 'ReactDOM.render' with a new
container, using a key or a different type, after the contents of the
first container were messed with outside of React. This case throws,
and now at least there will be an informative warning along with the
error.
- We updated the check to compare the parent of the 'hostInstance' to
the container; this seems less fragile
- tweaked some comments
**why make this change?:**
Continue improving this to make it more final.
**test plan:**
`yarn test`
**issue:**
https://github.com/facebook/react/issues/8854
* Stub our `console.error` in one of the portal tests
**what is the change?:**
See title
**why make this change?:**
See comment in the code
**test plan:**
`yarn test`
**issue:**
https://github.com/facebook/react/issues/8854
* Skip warning in 'ReactDOMFiberEntry' when mounting to Comment node
**what is the change?:**
We have a warning for cases when the container doesn't match the parent
which we remembered the previously rendered content being rendered into.
We are skipping that warning when you render into a 'comment' node.
**why make this change?:**
Basically, if you render into a 'comment' node, then the parent of the
comment node is the container for your rendered content. We could check
for similarity there but rendering into a comment node seems like a
corner case and I'd rather skip the warning without knowing more about
what could happen in that case.
**test plan:**
`yarn test`
**issue:**
https://github.com/facebook/react/issues/8854
* Improve warning message, remove dedup check, and unmock console.error
**what is the change?:**
Various changes to get this closer to being finished;
- Improved warning message (thanks @spicyj!!!)
- Removed dedup check on warning
- Remove mocking of 'console.error' in portals test
**why make this change?:**
- warning message improvement: communicates better with users
- Remove dedup check: it wasn't important in this case
- Remove mocking of 'console.error'; we don't want to ignore an
inaccurate warning, even for an "unstable" feature.
**test plan:**
`yarn test` -> follow-up commits will fix the remaining tests
**issue:**
issue #8854
* Possible fix for issue of incorrect warning for portal re-render
**what is the change?:**
Add a property to a container which was rendered into using
`ReactDOM.unstable_createPortal`.
**why make this change?:**
We don't want to warn for mismatching container nodes in this case - the
user intentionally rendered into the portal container instead of the
original container.
concerns;
- will this affect React Native badly?
- will this add bloat to the portal code? seems small enough but not
sure.
**test plan:**
`yarn test`
**issue:**
https://github.com/facebook/react/issues/8854
* Fix logic for checking if the host instance container is a portal
**what is the change?:**
When focusing on fixing the warning to not check when we are using
portals, I missed checking for the existence of the host instance parent
before checking if it was a portal. This adds the missing null checks.
**why make this change?:**
To fix a bug that the previous commit introduced.
**test plan:**
`yarn test`
-> follow-up commits fix more of the test failures
**issue:**
https://github.com/facebook/react/issues/8854
* Clean up new tests in ReactDOMFiber-test
**what is the change?:**
- removed extra single quotes, downgrade double quotes to single
- update expected warning message to match latest warning message
- fix indentation
**why make this change?:**
- get tests passing
- code maintainability/readability
**test plan:**
`yarn test`
follow up commits will fix the remaining tests
**issue:**
https://github.com/facebook/react/issues/8854
* Add 'unmountComponentAtNode' call in test for reconciling pre-rendered markup
**what is the change?:**
We have a test that verifies React can reconcile text from pre-rendered
mark-up. It tests React doing this for three strings and three empty
strings.
This adds a call to 'unmountComponentAtNode' between the two
expectations for strings and empty strings.
**why make this change?:**
We now warn when someone messes with the DOM inside of a node in such a
way that removes the React-rendered content. This test was doing that. I
can't think of a situation where this would happen with server-side
rendering without the need to call 'unmountComponentAtNode' before
inserting the server-side rendered content.
**test plan:**
`yarn test`
Only one more failing test, will fix that in the next commit.
**issue:**
https://github.com/facebook/react/issues/8854
* ran prettier
* remove unused variable
* run scripts/fiber/record-tests
* Fix type error and improve name of portal container flag
**NOTE:** I am still looking for a good place to move this flag
assignment to, or a better approach. This does some intermediate fixes.
**what is the change?:**
- fixed flow error by allowing optional flag on a DOMContainer that
indicates it was used as a portal container.
- renamed the flag to something which makes more sense
**why make this change?:**
- get Flow passing
- make this change make more sense
We are still not sure about adding this flag; a follow-up diff may move
it or take a different approach.
**test plan:**
`yarn test`
**issue:**
https://github.com/facebook/react/issues/8854
* Add flag to portalContainer on mount instead of in `createPortal`
**what is the change?:**
We add a flag to the container of a 'portal' in the 'commit work' phase
in Fiber. This is right before we call `appendChildToContainer`.
**why make this change?:**
- Sometimes people call `ReactDOM.render(... container)`, then manually
clear the content of the `container`, and then try to call another
`ReactDOM.render(... container)`.
- This leads to cryptic errors or silent failure because we hold a
reference to the node that was rendered the first time, and expect it
to still be inside the container.
- We added a warning for this issue in `renderSubtreeIntoContainer`, but
when a component renders something returned by
`ReactDOM.unstable_createPortal(<Component />, portalContainer);`,
then the child is inside the `portalContainer` and not the `container,
but that is valid and we want to skip warning in that case.
Inside `renderSubtreeIntoContainer` we don't have the info to determine
if a child was rendered into a `portalContainer` or a `container`, and
adding this flag lets us figure that out and skip the warning.
We originally added the flag in the call to
`ReactDOM.unstable_createPortal` but that seemed like a method that
should be "pure" and free of side-effects. This commit moves the
flag-adding to happen when we mount the portal component.
**test plan:**
`yarn test`
**issue:**
https://github.com/facebook/react/issues/8854
* Force an 'any' type for the `hostInstance.parentNode` in warning check
**what is the change?:**
This is awful. :(
I'm not sure how else to let Flow know that we expect that this might be
a sort of `DOMContainer` type and not just a normal `Node` type.
To at least make the type information clear we added a comment.
**why make this change?:**
To get `flow` passing. Looks like we have `any` types sprinkled
throughout this file. phooey. :(
**test plan:**
`yarn flow`
**issue:**
https://github.com/facebook/react/issues/8854
* Ignore portals in `DOMRenderer.findHostInstance`
**what is the change?:**
We want to ignore portals when firing a certain warning.
This allows us to get the host instance and ignore portals.
Also added a new snapshot recording while fixing things.
**why make this change?:**
Originally we had added a flag to the DOM node which was used for
rendering the portal, and then could notice and ignore children rendered
into those nodes.
However, it's better to just ignore portals in
`DOMRenderer.findHostInstance` because
- we will not ignore a non-portal second child with this approach
- we meant to ignore portals in this method anyway (according to a
'TODO' comment)
- this change only affects the DOM renderer, instead of changing code
which is shared with RN and other renderers
- we avoid adding unneeded expandos
**test plan:**
`yarn test`
**issue:**
https://github.com/facebook/react/issues/8854
* Ran prettier
* Remove error snapshot test
I think there is a bug where an empty snapshot is treated as an 'outdated snapshot'.
If I delete the obsolute snapshot, and run ReactDOMFiber-test.js it generates a new snapshot.
But then when I run the test with the newly generated snapshot, it says "1 obsolete snapshot found",
At some point I will file an issue with Jest. For now going to skip the snapshot generation for the error message in the new test.
* Remove expando that we were adding to portal container
**what is the change?:**
see title
**why make this change?:**
this is part of an old approach to detecting portals, and we have
instead added a check in the `findHostInstance` method to filter out
portals.
**test plan:**
`yarn test`
**issue:**
https://github.com/facebook/react/issues/8854
* Fork `findHostInstance` to make `findHostInstanceWithNoPortals`
**what is the change?:**
We need to get host instances, but filter out portals. There is not
currently a method for that.
**why make this change?:**
Rather than change the existing `findHostInstance` method, which would
affect the behavior of the public `findDOMNode` method, we are forking.
**test plan:**
`yarn test`
**issue:**
https://github.com/facebook/react/issues/8854
Alternative to using the static class property unstable_asyncUpdates.
Everything inside <AsyncComponent /> has async updates by default. You
can also extend it like PureComponent or Component.
* Disable Fiber specific test run in CI
This disables the comparison against previously recorded test. Instead,
we'll rely on jest failures to fail tests.
* Extract jest config into two separate projects for Fiber and Stack
Allows us to run both in the same jest run. The setupMocks file is forked into
specific environment configuration for each project. This replaces the
environment variable.
I used copy pasta here to make it clear. We can abstract this later. It's clear
to me that simply extracting shared stuff is not the best way to abstract this.
setupMocks for example didn't need all the code in both branches.
I think that some of the stuff that is shared such as error message extracting
etc. should probably be lifted out into a stand-alone jest project instead of
being shared.
* Fix class equivalence test
There's a behavior change when projects are used which makes
setupTestFrameworkScriptFile not override the normal config.
This test should probably just move to a separate CI script or something
less hacky.
* Only run Fiber tests with scripts/fiber/record-tests
* Move input valueTracker to DOM nodes
This moves the storage of the input value tracker to the DOM node
instead of the wrapperState. This makes it easier to support both Stack
and Fiber without out a lot of ugly type casting and logic branches.
related: #10207
* run prettier
* remove instance accepting methods
* rm unused trst method
* address feedback
* fix naming
* fix lint
* Replace SSR unit test with integration test
* Remove unit test that is already covered by integration suite
* Replace unit tests for boolean attrs with integration tests
* Replace unit test for aria attrs with integration test
* Replace unit tests for numeric 0 with integration tests
* Remove unit test covered by integration tests
* Replace unit test for injection with integration test
It still touches internals but it tests both renderers.
* Fork DOMPropertyOperations into DOMMarkupOperations
* Trim down DOMPropertyOperations and DOMMarkupOperations
* Record SSR sizes
* Record them tests
* Fix false positive warning for overloaded booleans when passing numbers
* Remove stray import
* Replace CSS markup tests with public API tests
Some of these are handy as integration tests so I moved them there.
But some test markup specifically so I changed them to use DOMServer.
* Make CSSPropertyOperations client-only
I forked createMarkupForStyles() into ReactDOMComponent and ReactPartialRenderer. Duplication is fine because one of them will soon be gone (guess which one!)
The warnInvalidStyle helper is used by both server and client, unlike other client-only stuff in CSSPropertyOperations, so I moved it to a separately module used in both.
* Record server bundle size
* Add an early exit to validation
* Clarify what is being duplicated
* Upgrade jest to 20.1.0-delta.1
This includes multi-project support.
* Use isSpy polyfill that is not available in jest 20
* Remove use of jasmine.createSpyObj
We don't really need this and it's not in jest 20.
* Upgrade record-tests script to use the new jest 20 APIs
* Use performFailedUnitOfWork
instead of beginFailedWork and completeUnitOfWork separately. Also, we
unwind the context stack *before* beginning work again.
* Only use error loop directly after a commit
We have a special, forked version of work loop that checks if a fiber is
in a failed state (needs to be unmounted). This is only relevant right
after a commit -- begin phase errors are handled differently, by
unwinding the stack.
Also renamed findNextUnitOfWork to resetNextUnitOfWork and made it a
void function to signal that it's impure.
* Reset nextUnitOfWork after every commit
* Include the error boundary when unwinding a failed subtree
Also added a warning to the error boundary to show that it failed.
* Push context providers in beginFailedWork to avoid push/pop mismatch
Added a test that demonstrates how an error boundary that is also a
context provider could pop its context too many times if you neglect
to push it in beginFailedWork. This happens because we've already
popped the context once in unwindContext.
The solution is a code smell. I don't like how we push/pop context in
so many places. Shouldn't they all happen in the same location?
* Refactor work loop
- Optimizes the normal, non-error path by reducing the number of checks
needed to begin performing work.
- Prevents control flow from oscillating between fast normal loop and
slower error loop.
* Improve context unwinding test
Tests that we correctly unwind an error boundary that is also a
context provider.
* Triangle tester should assert the tree is consistent after every action
...not just at the end.
* Better implementation of infinite loop error
Infinite loops should only be possible if a while loop never terminates.
Now that we've refactored to avoid oscillation between different
work loops, we can count updates local to each loop.
The two loops that could infinite loop are the sync work loop and the
loop that surrounds the body of the render phase catch block. The
async loop could also fall into an infinite loop if the deadline never
terminates, but we'll assume that it always eventually does.
This change also creates better error stack traces because the error is
thrown from inside the first setState that exceeds the limit.
Added a test case for an error boundary whose parent remounts it
on recovery.
* Use invokeGuardedCallback in DEV
* Add a regression test that passes in Stack but fails in Fiber
The failure happens because trackValueOnNode() can exit early if it detects an existing descriptor on node, or if it sees a "broken" Safari descriptor (which is how we encountered this bug in the wild).
As a result, the tracker field was not set, and subsequent updateValueIfChanged() call went into the branch that initializes the tracker lazily. That branch has a bug in Fiber mode where it passes the wrong type.
We did not see this issue before because that branch is relatively hard to hit (you have to either run it in Safari or define a custom DOM value descriptor which is what I did in the test).
In the future, we will likely remove the lazy branch altogether since if we bailed out of setting up the tracker once, we will likely bail out every time. But for now I'm just focused on a minimal fix to unbreak master.
* Fix updateValueIfChanged() lazy path to work with DOM argument
* Slightly reorder lines for clarity and record tests
* Also test the change event code path
This makes it go through the Fiber argument code path.
* Extract the top element frame from ReactDebugCurrentFrame
This is part of a larger refactor to decouple stack addendums. All
renderers have their own way of getting the stack of the currently
executing components.
There is one special case in Element Validator that adds an additional line
for the element being validated. This commit moves that special case in
into the validator.
There is another case where it looked like this was used in shallow
renderer but this is actually something different. It is part of the
component stack. It just happens to be that shallow renderer has a simpler
implementation of the component stack that just happens to be a single
element.
This will let us decouple the implementation to get a stack from
ReactDebugCurrentFrame and put that in each renderer.
* Stop using ReactComponentTreeHook for Fiber
Currently we fall back to ReactCurrentOwner in ReactComponentTreeHook for
stack addendums. We shouldn't need to because we should use
ReactDebugCurrrentFiber.
Ensure we always set both ReactDebugCurrentFiber and ReactDebugCurrentFrame
so that we can rely on these for all stacks.
* Make ReactDebugCurrentFrame implementation independent
Introduced ReactDebugCurrentStack for the Stack renderer which does the
same thing as ReactDebugCurrentFiber.
ReactDebugCurrentFrame no longer keeps track of the current fiber/debug id.
That's handled by the individual renderers.
Instead, it is now used to keep track of the current *implementation* of
the current stack frame. That way it is decoupled from the specifics of
the renderers. There can be multiple renderers in a context. What matters
is which one is currently executing a debuggable context (such as a render
function).
* Add debug frames to ReactPartialRenderer (ssr)
Basic functionality.
* Add shared modules to shallow renderer
This is now needed because we share describeComponentFrame.
* Limit the number of nested synchronous updates
In Stack, an infinite update loop would result in a stack overflow. This
gives the same behavior to Fiber.
Conceptually, I think this check belongs in findNextUnitOfWork, since
that is what we call right before starting a new stack. I've put it
in scheduleUpdate for now so I have access to the component that
triggered the nested update. But we could track that explicitly instead.
I've chosen 1000 as the limit rather arbitrarily. Most legit use cases
should really have a much smaller limit, but a smaller limit could break
existing code. For now I'm only concerned with preventing an infinite
loop. We could add a warning that fires at the smaller limit.
* Move check to findNextUnitOfWork
Including the name of the component in the message probably isn't
necessary. The JS stack will include either componentDidUpdate or
componentWillUpdate. And the component that's updating won't
necessarily be the component whose lifecycle triggered it.
So let's move the infinite loop check to findNextUnitWork as I
originally wanted to.
* WIP Improve error message thrown in Fiber with multiple copies of React
**what is the change?:**
Adding an 'invariant' with detailed error message for the problem that
occurs when you load two copies of React with the Fiber reconciler.
WIP:
- Is there any other likely cause for this error besides two copies of
React?
- How can we make the message more clear?
Still TODO:
- Write a unit test
- Write a documentation page and create the link to the page, similar
to https://facebook.github.io/react/warnings/refs-must-have-owner.html
It would also be nice to have a page with instructions on how to fix
common cases of accidental double-loading of React, but we can open a
separate issue on that and let the community do it.
**why make this change?:**
This error comes up relatively often and we want to make things clear
when it happens in v16.0+
**test plan:**
WIP
**issue:**
Fixes#9962
* Add improved warning and docs for 'refs must have owner' in Fiber
**what is the change?:**
- Added warning in the place where this error is thrown in Fiber, to
get parity with older versions of React.
- Updated docs to mention new error message as well as old one.
I started to write a new docs page for the new error, and realized the
content was the same as the old one. So then I just updated the existing
error page.
**why make this change?:**
We want to avoid confusion when this error is thrown from React v16.
**test plan:**
- manually inspected docs page
- manually tested in a CRA to trigger the error message
(Flarnie will insert screenshots)
**issue:**
Fixes#9962
Related to #8854
* Add test for the informative warning around multiple react copies
@gaearon debugged the test for this and now it works!!!!!!!!!!!!!!!
!!!!!!!!!!!!!!!!!!!!!!!!!!!!! :)
**what is the change?:**
We now test for the warning that the previous commits add in Fiber, and
also test for the old warning in the stack reconciler.
**why make this change?:**
This is an especially important warning, and we want to know that it
will fire correctly.
**test plan:**
`yarn test src/renderers/__tests__/multiple-copies-of-react-test.js`
`REACT_DOM_JEST_USE_FIBER=1 yarn test src/renderers/__tests__/multiple-copies-of-react-test.js`
* Fix up test for 'multiple copies of react'
**what is the change?:**
refactor test for 'multiple copies of React' to be simpler and remove
some copypasta
* run prettier
* Fix conditionals in 'multiple copies of react' test
**what is the change?:**
When moving the 'fiber' and 'non-fiber' conditions from two assertions
into one, we copy pasted the wrong message into the 'fiber' condition.
This wasn't caught because we were using an outdated name for the
'fiber' constant when running the tests locally with fiber enabled.
This fixes the copy-paste error and we now are actually running the
tests with fiber enabled locally.
* Run scripts/fiber/record-tests
* change the argument passed to CallbackQueue.getPooled
* remove undefined from function call
* add test for ReactNativeReconcileTransaction
* update log of tests
* change test to one that operates on setState
* added new tests and fixed another instance of the bug
* run prettier
* update names of tests and minor clean up
* remove arg from CallbackQueue and update tests
* Add react-dom-unstable-native-dependencies
react-native-web and react-primitives currently access a few internals
for shimming DOM events into native ones. Changes in react@16 packaging
hide these internals completely. This change adds a submodule to react-dom,
unstable-native-dependencies that includes the necessary modules to
continue enabling that method of dom-native event injection.
* Update ResponderEventPlugin to use "public" interfaces for test
In order to get some sort of smoke testing on
react-dom-unstable-native-dependencies, update ResponderEventPlugin-test
to use the "public" interfaces provided by react-dom and the new
react-dom/unstable-native dependencies
Also adds the missing references in package.json as well as missing
files required for unittests to do imports correctrly
Also exports injectComponentTree() which is required for the unittests
to re-set the shared component state between runs.
* Tweak bundle comment
* Bundle content updates from exporting injectComponentTree
* Added FB_DEV, FB_PROD to bundle types
* Run yarn prettier for -unstable-native-dependencies updates
* Add failing test to show that shallow test renderer doesn't call setState's callback arg
* Record tests
* Fix shallow renderer's setState/replaceState/forceUpdate to execute any callbacks passed. (#10089)
* Ensure shallow renderer callbacks are called with the correct binding.
To make sure we don't reset the priority of a down-prioritized fiber,
we compare the priority we're currently rendering at to the fiber's
work priority. If the work priority is lower, then we know not to reset
the work priority.
* Warn for text content
* Warn for different attributes/properties values
Warns if there are unknown extra attributes in the hydrated node.
It also tries to compare the existing property or attribute against the
expected value. It does this by reading the property and comparing it to
the prop. Except it's not that simple because multiple prop values can
yield the same output. For this we pass an extra expected value that is
a hint as to which one was used. This is a bit weird but I'm not sure the
alternatives were much better.
* Warn when there is an insertion or deletion during hydration
This warns if there is ever an insertion or deletion due to hydration
failing to find a match.
Currently we can't warn for insertions required into the root because
that's how we do all non-hydrating renders atm. Left a todo.
This strategy is a bit unfortunate that it leads to so much plumbing code.
And we have to add three extra methods to the HostConfig that are only used
in DEV and not for anything else. I don't really have a better idea.
* Don't try to delete children of a textarea
Textareas are special cases. The initial mount inserts children
as the default value, but we should leave that untouched. This is the same
as the special case where we set text content of children so I'll use that
mechanism.
* Change expected format for text differences
In Stack this is presented as HTML which needs to have normalized escaping
rules. In Fiber it is currently not presented as HTML but a raw string
so we don't escape it.
* Unmount component in between tests
In Fiber, the second warning isn't issued because it's considered an update
not a new initial render and we don't fire the warning for those.
* Change expectation of white space text behavior in Fiber
In Fiber we don't expect empty strings to be different from rendering null.
In fact, in a follow up I plan on formalizing this by never creating text
Fibers for empty strings.
* Warn for different dangerouslySetInnerHTML
We can't just compare the raw innerHTML value because it will have been
normalized. Instead, we'll create another element, set its innerHTML and
read it back.
Since there can be custom elements registered with this document, we want
to avoid any side-effects they might cause. So I do this in a fresh new
document.
I'm not sure how this would affect controlled components and other stuff
that could have changed after runtime. I think for those cases maybe we
just need a general way of opting out of the diff.
* Warn if unmounting a non-container
* Warn if the 2nd+ child is a "root" element but not first
This triggers our non-reuse mode. This is covered by ReactMount already but
the test doesn't pass yet without also landing #10026.
This covers up errors that are thrown in Fiber, because callback gets
fired *and* an error is thrown. Created a follow up #10049 to reevaluate
these semantics.
# Conflicts:
# scripts/fiber/tests-passing-except-dev.txt
# scripts/fiber/tests-passing.txt
There's no advantage to scheduling updates with animation priority
versus scheduling sync work inside requestAnimationCallback. So we can
remove all the animation-specific code. Now there's only one type of
callback.
Allows us to make assertions on the values that are yielded when
performing work. In our existing tests, we do this manually by pushing
into an array.
ReactNoop.flushThrough extends this concept. It accepts an array of
expected values and flushes until those values are yielded.
* Prevents adding units to css custom properties
* Fix code style
* Optimize custom property checking
* Prevents adding units to css custom properties in markup creation
* Update passing tests
* Fix argument name and reuse check in DEV
* Remove internal forwarding modules for /lib/
* Add *Entry suffix to all entry points
* Don't bundle ReactNativeFeatureFlags since it's shimmed
* Delete TestRendererStack
* Switch tests at forwarding modules rather than via Jest
* Share mocks between regular and equivalence fixtures
* Rename environment flag to be more generic
* Remove accidental variable name change
* Minor naming changes for consistency
Files that have two versions get the engine in variable name.
