-
Notifications
You must be signed in to change notification settings - Fork 46.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Accept promise as element type #13397
Conversation
Resolving |
ReactDOM: size: 🔺 2.2%, gzip: 🔺 1.7% Details of bundled changes.Comparing: 5816829...9c55b30 react
react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
Generated by 🚫 dangerJS |
This use case is literally the point of default exports. To provide a conceptual main item among other items so that it can be picked automatically. There is no benefit to not resolving it, since you gain no syntactical benefit when you don't use them. It's still the same bloated syntax to load a named export whether this exists or not. Other than artificially encourage the more bloated form. |
(current.type === elementType || | ||
(elementType !== null && | ||
elementType !== undefined && | ||
current.type === elementType._reactResult)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super hot. This means that when we reconcile in a long list, we have to do several comparisons and a missing property lookup (which can't be optimized in this function since it's a new hidden class for each one, and traverses the prototype chain all the way up to Object.prototype).
We also don't typically ensure that wrappers get unwrapped to reconcile their inner value so I don't think that we need to semantically support the case where you switch from an instance of the component to the promise of that same component.
I'd say that its worth the complexity elsewhere to allow for fiber.type
to be the thennable.
const key = element.key; | ||
let pendingProps = element.props; | ||
|
||
if (type !== null && type !== undefined && typeof type.then === 'function') { | ||
type = resolveThenableType(type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a subtle issue with this approach. Since this throws when the parent reconciles, there is no way to continue siblings of the component that is lazy even if they're available. By making it throw in the beginWork of the individual component, we avoid that problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a test for that. Works by creating a new component type and throwing in beginWork, as you say. Not the greatest, but the new approach we discussed offline will be better.
Don't forget to deal with defaultProps. |
6aa0996
to
6103fc0
Compare
@sebmarkbage Updated |
Component, | ||
renderExpirationTime, | ||
); | ||
workInProgress.pendingProps = workInProgress.memoizedProps = props; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't decide if you would hate this or not :D
return false; | ||
} | ||
const childContextTypes = type.childContextTypes; | ||
return childContextTypes !== null && childContextTypes !== undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, meant to come back to this function. I'll find a way to get rid of it.
export const ContextConsumer = 11; | ||
export const ContextProvider = 12; | ||
export const ForwardRef = 13; | ||
export const ForwardRefLazy = 14; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DevTools won't be happy about this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DevTools needs to get its act together. This is important to ensure that VMs don't decide to avoid the jump table by thinking there are too many gaps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there perhaps a way to expose this file to devtools, that way it can find & read it, as to not break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that hardcoding this in devtools based on version is probably the way to go. It doesn't require us to include extra strings and object initialization in the production builds.
I suspect that even the fiber fields might be mangled in the future so it'll need the same parity.
If we move to another language in the future, that language likely don't let us define our own indices. It'll be a header file or something that determines this based on compiler settings resulting in a reified contract. To make that work you typically have to compile both sides from the same source with the same settings. E.g. the devtools would have one header file per version.
That's the equivalent of the devtools just keeping a copy of this file per version. Maybe we should do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since 16 we inject the version into DevTools so yeah, we can do this.
It's very exciting to see this PR. Lazy load component will be easier. Are there any planned accepting observable as element type? Or something like rxjs-react(My experiment library which supports every part of an element to be observable) |
On the initial render, the element will suspend as if a promise were thrown from inside the body of the unresolved component. Siblings should continue rendering and if the parent is a Placeholder, the promise should be captured by that Placeholder. When the promise resolves, rendering resumes. If the resolved value has a `default` property, it is assumed to be the default export of an ES module, and we use that as the component type. If it does not have a `default` property, we use the resolved value itself. The resolved value is stored as an expando on the promise/thenable.
6c75678
to
2911577
Compare
export function getLazyComponentTypeIfResolved<T>( | ||
thenable: Thenable<T>, | ||
): T | null { | ||
return thenable._reactStatus === Resolved ? thenable._reactResult : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this should throw if it is not resolved. If it's not resolved, we can't reason about whether something will be one thing or another so we don't want to make assumptions that might be persisted.
When we do reason about it, we must throw a promise and terminate regardless.
In cases where we know it'll always be resolved, we want the return type to be specific so that we don't have to add unnecessary checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it this way because it's called from places that aren't allowed to throw. But I suppose I could wrap those call sites in a try-catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't wrap them in try/catch but maybe it's an indication that something else is problematic in those cases. Which ones are they? The issue now is that I can't be sure if something is expected to need a fallback or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I got rid of it
@@ -14,10 14,13 @@ import type {Instance, TextInstance} from './ReactTestHostConfig'; | |||
import * as TestRenderer from 'react-reconciler/inline.test'; | |||
import {batchedUpdates} from 'events/ReactGenericBatching'; | |||
import {findCurrentFiberUsingSlowPath} from 'react-reconciler/reflection'; | |||
import {getLazyComponentTypeIfResolved} from 'react-reconciler/src/ReactFiberLazyComponent'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should never deep link in renderers. This must go onto reflection if you want it. However, better yet is probably to duplicate the logic here. "But then it'll be inconsistent if we change something!" Yea, but that's only because this whole renderer is already a fork of a bunch of internal logic that will be inconsistent all the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, that was vscode's auto importer. Forget to fix. It works well if you already imported from that module, not as well for new imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my bad, I thought this was a different file. Ok.
@@ -30,7 30,8 @@ export default function isValidElementType(type: mixed) { | |||
type === REACT_PLACEHOLDER_TYPE || | |||
(typeof type === 'object' && | |||
type !== null && | |||
(type.$$typeof === REACT_PROVIDER_TYPE || | |||
(typeof type.then === 'function' || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put the .then
comparison last since it is the least common one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really? I intentionally put it first because it seems like it will be the most common one. Providers are pretty rare. Perhaps ForwardRef will more common, in the near term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, that's fair.
} | ||
return props; | ||
} | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling this, I would expect this to return baseProps
if there are no default props.
); | ||
} | ||
case ClassComponent: { | ||
const Component = workInProgress.type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You already pass the resolved type as an argument. I think it would be better to do the same thing with props.
const props = workInProgress.pendingProps;
return updateClassComponent(
current,
workInProgress,
Component,
props,
renderExpirationTime,
);
Then in the lazy form you do
const props = resolveDefaultProps(Component, workInProgress.pendingProps);
That way you don't need any special casing for null and separate updateClassComponentLazy
helper and such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll still need updateClassComponentLazy
to do the defaultProps
resolution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you just do it in the switch statement like how you resolve the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nvm I read it wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tricky thing is that I think I need to reset the memoized props back to the unresolved value so we can quickly bail out the next time.
EDIT: I guess this isn't so tricky.
if ( | ||
Component !== null && | ||
Component !== undefined && | ||
typeof Component.then === 'function' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should check typeof Component === 'object'
? Seems like a function with a .then
is ambiguous at best.
@@ -278,7 278,7 @@ const createFiber = function( | |||
return new FiberNode(tag, pendingProps, key, mode); | |||
}; | |||
|
|||
function shouldConstruct(Component) { | |||
export function shouldConstruct(Component: Function) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't make sense that other files import this function from ReactFiber. It doesn't have anything to do with the Fiber.
However, the way it is used, is to upgrade the tag. Maybe we could instead export a function like export function reassignTag(fiber: Fiber, Component: Function): TypeOfWork
that does the work of figuring out which tag should've been used.
Then in mountIndeterminateComponent you use the resulting tag to figure out which branch to take.
That way, the logic about picking tag is fully encapsulated in ReactFiber.
@@ -388,7 388,7 @@ export function createFiberFromElement( | |||
} | |||
|
|||
let fiber; | |||
const type = element.type; | |||
let type = element.type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not const
? You're not reassigning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it has fewer characters, obviously.
@@ -450,6 450,13 @@ function getFiberTagFromObjectType(type, owner): TypeOfWork { | |||
case REACT_FORWARD_REF_TYPE: | |||
return ForwardRef; | |||
default: { | |||
if ( | |||
type !== undefined && | |||
type !== null && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we shouldn't need to check these. Given that this function is called ...FromObjectType
, I think we've already assumed that it is indeed an object. If not, should we? At least we can return early at the top of this function instead of checking this twice.
} | ||
case ClassComponentLazy: { | ||
const thenable = workInProgress.type; | ||
const Component = (thenable._reactResult: any); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we wrap all reads of _reactResult
in a helper. Like ReactFiberLazyComponent.readResolvedType(workInProgress)
. Normally I wouldn't recommend that but in this case it is a user space object. We might want to add assertions or warnings in the future. It is also a indicator that we can use that this is external to Fiber and will have inconsistent hidden classes. It might just be a a few hidden classes and it's good that those optimizations get to use the same inline cache if possible rather than redoing that work at every callsite.
It also allows us to return a more specific type than any.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about ReactTestRenderer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test renderer doesn't need to.
); | ||
} | ||
} | ||
workInProgress.pendingProps = props; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pending props needs to match the memoized props so that the next time we visit this fiber, when we compare memoizedProps
to pendingProps
, they are the same object.
Although this looks like a mistake. I think it should be workInProgress.memoizedProps = props
. Clearly I forgot to write a test... will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't make sense because memoizedProps on other paths stores the unresolved props. I think it needs to store the unresolved props because if you store the resolved ones, you'll never get reference equality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, I did write a test. But if I delete it, it passes. I think it was failing in an earlier version.
It's not needed because we compare to current's pendingProps
instead of memoizedProps
. Which kinda suggests we should delete one of those fields :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it needs to store the unresolved props because if you store the resolved ones, you'll never get reference equality.
Yeah, that's what this was originally intended to do: restore the unresolved props.
Removes unnecessary checks.
7217123
to
4ab2ead
Compare
packages/shared/getComponentName.js
Outdated
import { | ||
refineResolvedThenable, | ||
getResultFromResolvedThenable, | ||
} from 'react-reconciler/src/ReactFiberLazyComponent'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what the rules are for deep linking from the shared
directory... I can inline the helpers if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look good. If those are meant to be sharable then they should be in shared
themselves. Like TypeOfWork
.
break; | ||
} | ||
case ForwardRefLazy: { | ||
child = updateForwardRef( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can turn all of these into early returns now.
); | ||
} | ||
|
||
export function reassignLazyComponentTag( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slight preference for returning the tag and assigning it in the outer function to avoid an unnecessary Get since this will be inlined.
thenable._reactStatus = Resolved; | ||
// If the default value is not empty, assume it's the result of | ||
// an async import() and use that. Otherwise, use resolved value. | ||
const defaultExport = (resolvedValue: any).default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is claiming the .default
property name on Function.prototype etc. I wonder if we should be more specific and check for typeof object on the resolvedValue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use resolvedValue.__esModule ? resolvedValue.default : resolvedValue
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't __esModule
just a Babel thing? I don't think it's in the standard. So e.g. Node might not have it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct, Node.js with --experimental-modules
doesn't have it,and neither does Chrome's native ESM support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nits left. Just make sure to fix that getComponentName thing.
4ab2ead
to
419359f
Compare
419359f
to
c26a3d4
Compare
@acdlite Will you prepare a DevTools PR that forks the type of work list? |
@gaearon Uh sure but you'll have to give me more details :D I have zero clue how DevTools works. |
You can search for uses of this file: https://github.com/facebook/react-devtools/blob/master/backend/ReactTypeOfWork.js Will need to check the version we inject into DevTools and use the new file if it’s fresh enough. |
} | ||
|
||
function isContextProvider(fiber: Fiber): boolean { | ||
return fiber.tag === ClassComponent && fiber.type.childContextTypes != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tag
check was important here.
The code in getUnmaskedContext
above says:
const hasOwnContext = isContextProvider(workInProgress);
if (hasOwnContext) {
// If the fiber is a context provider itself, when we read its context
// we have already pushed its own child context on the stack. A context
// provider should not "see" its own child context. Therefore we read the
// previous (parent) context instead for a context provider.
return previousContext;
}
However, for factory components we used to execute getUnmaskedContext
before pushing own context on the stack (because we don't know own context yet). In fact we called it twice: once before calling the factory, and once after we know it's a factory component.
The first call used to be ignored because isContextProvider
was "unsure" when given IndeterminateComponent
and returned false
. However now isContextProvider
is more confident when IndeterminateComponent
is a context provider. Breaking the assumption that getUnmaskedContext()
isn't called with a context provider until after it's been pushed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix in #13441
I'm updating DevTools in facebook/react-devtools#1101 to unblock the sync. |
const thenable: Thenable<mixed> = (type: any); | ||
const resolvedThenable = refineResolvedThenable(thenable); | ||
if (resolvedThenable) { | ||
const Component = getResultFromResolvedThenable(resolvedThenable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is like type._reactResult._reactResult
. Unintentional?
DevTools support: facebook/react-devtools#1103 |
* Accept promise as element type On the initial render, the element will suspend as if a promise were thrown from inside the body of the unresolved component. Siblings should continue rendering and if the parent is a Placeholder, the promise should be captured by that Placeholder. When the promise resolves, rendering resumes. If the resolved value has a `default` property, it is assumed to be the default export of an ES module, and we use that as the component type. If it does not have a `default` property, we use the resolved value itself. The resolved value is stored as an expando on the promise/thenable. * Use special types of work for lazy components Because reconciliation is a hot path, this adds ClassComponentLazy, FunctionalComponentLazy, and ForwardRefLazy as special types of work. The other types are not supported, but wouldn't be placed into a separate module regardless. * Resolve defaultProps for lazy types * Remove some calls to isContextProvider isContextProvider checks the fiber tag, but it's typically called after we've already refined the type of work. We should get rid of it. I removed some of them in the previous commit, and deleted a few more in this one. I left a few behind because the remaining ones would require additional refactoring that feels outside the scope of this PR. * Remove getLazyComponentTypeIfResolved * Return baseProps instead of null The caller compares the result to baseProps to see if anything changed. * Avoid redundant checks by inlining getFiberTagFromObjectType * Move tag resolution to ReactFiber module * Pass next props to update* functions We should do this with all types of work in the future. * Refine component type before pushing/popping context Removes unnecessary checks. * Replace all occurrences of _reactResult with helper * Move shared thenable logic to `shared` package * Check type of wrapper object before resolving to `default` export * Return resolved tag instead of reassigning
On the initial render, the element will suspend as if a promise were thrown from inside the body of the unresolved component. Siblings should continue rendering and if the parent is a Placeholder, the promise should be captured by that Placeholder.
When the promise resolves, rendering resumes. If the resolved value has a
default
property, it is assumed to be the default export of an ES module, and we use that as the component type. If it does not have adefault
property, we use the resolved value itself.The resolved value is stored as an expando on the promise/thenable.