* Don't catch the assertion about missing errors
Previously, even if an error was not thrown, the assertion about this was ignored.
This fix exposes two failing cases in new SSR that weren't failing in Stack.
* Add child context validation to new SSR
This fixes the error cases that were silently failing before but are now exposed.
* fix(*): fix renderToString fails with array type children when react-dom/server render
* Add an integration test that verify renderToString with array type children
* Add integration test to renderToString with array type child
* Update integration test to renderToString with array type child
* fix renderToString fails with array type children when react-dom/server render
* Update integration test to renderToString with array type child
* Add to iterate that are not arrays
* Add the validation react element
* Improve an integration test of server renderToString with array type
* prettier
* prettier
* Make SSR can handle a single array element and nested array
* prettier
* Change integeration test description
* modified: src/renderers/dom/ReactDOMNodeStreamRenderer.js
* Make invariants consistent
* Gate outdated test itself by feature flag
* Change test to make sense both with old and new code
* Test more cases
* Add date time test fixtures
This commit adds a new section for the DOM test fixtures specifically
for date inputs. Additionally, it adds a test case to verify that
correct transference of dates between the `"date"` and
`"datetime-locale"` input types.
* Adjust date parsing to be clearer
The critical semantics are resilient to browser flakiness, so we don't
need this feature test.
Also added comments explaining how invokeGuardedCallback dev works.
* Deduplicate unknown DOM tag warning
* Don't warn about <time> because it is widely used
Other browsers implement it, and Chrome will ship it soon too.
* Use hasOwnProperty
* Fix corner case: hasOwnProperty as a tag
* Update comments to be more correct factually
invokeGuardedCallback is a function we use in place of try-catch
statement. It accepts a function, and if the function throws, it
captures the error. In production, the implementation is a normal try-
catch. In development, we swap out the prod implementation for a special
version designed to preserve "Pause on all exceptions" behavior of the
browser DevTools.
invokeGuardedCallbackDev works by dispatching an event to a dummy DOM
node and calling the provided function inside a handler for that event.
We also attach an error event handler to the window object. If the
function throws, the global event handler is called and we can access
the error.
The global event handler is added and removed right before and after the
fake event is dispatched. But if invokeGuardedCallbackDev is nested --
that is, if it's invoked inside the body of another
invokeGuardedCallbackDev -- multiple error event handlers will attached
simultaneously. We only want the handler that corresponds to the deepest
level to handle the error. So we keep track of a depth counter, and
within the event handler, we only handle the error if the current depth
matches the depth at the time the function was invoked.
The problem that we discovered, and that this PR fixes, is that the
depth counter is local to each renderer. So if you nest separate copies
of invokeGuardedCallback from separate renderers, each renderer will
have its own depth counter, and multiple error handlers will fire for a
single, nested error.
* Reset modules between importing client and server modules in the test
Client and server renderers still share some modules, including injections.
This means they have shared state in this test, potentially missing issues that would occur in real world.
After this change, the client and server modules are completely isolated in the test.
This makes the tests fail due to https://github.com/facebook/react/issues/10265 (a real issue that was missed).
* Don't validate events if no plugins are injected
* Ran prettier over non-modified files to change them
* Fixed output of failing Prettier message to show invalid files
* Failing Prettier command now suggests 'yarn prettier-all'
* 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.
* Delete flattenChildren
* Stop exporting mapIntoWithKeyPrefixInternal
* Remove unescapeInDev()
flattenChildren() was the only traverseAllChildren() caller that relied on it.
In DEV mode this keeps an additional sub-stack within the stack of the
server renderer. This substack keeps track of all the elements that we
resolved to get to to the next processing frame.
This lets us recreate the full composite component stack for warnings.
Normally the stack only contains host components.
We reset this every time we're going to resolve another sibling.
* 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
Toru Kobayashi
Dan Abramov
guoyong yi
Dominic Gannaway
Fernando Montoya
Nathan Hunzaker
Dan Abramov
Sebastian Markbåge
Daniel Lo Nigro
Keyan Zhang
Brian Vaughn
Andrew Clark
Flarnie Marchan
Jason Quense