Skip to content
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

Merged
merged 14 commits into from Aug 16, 2018
Merged

Conversation

acdlite
Copy link
Member

@acdlite acdlite commented Aug 14, 2018

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.

@TrySound
Copy link
Contributor

TrySound commented Aug 14, 2018

Resolving default field feels out of react scope. Sure it's easier to combine with import(), but It will forces users to stick with default export just because it works out of the box. I found using named exports more productive.

@pull-bot
Copy link

pull-bot commented Aug 14, 2018

ReactDOM: size: 🔺+2.2%, gzip: 🔺+1.7%

Details of bundled changes.

Comparing: 5816829...9c55b30

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +0.9% +0.7% 58.72 KB 59.26 KB 16.45 KB 16.57 KB UMD_DEV
react.development.js +1.0% +0.8% 52.91 KB 53.46 KB 14.63 KB 14.74 KB NODE_DEV
React-dev.js +1.1% +0.8% 50.57 KB 51.12 KB 13.99 KB 14.1 KB FB_WWW_DEV

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +1.5% +1.0% 642.08 KB 651.4 KB 151.31 KB 152.85 KB UMD_DEV
react-dom.production.min.js 🔺+2.2% 🔺+1.7% 95.07 KB 97.18 KB 30.9 KB 31.41 KB UMD_PROD
react-dom.development.js +1.5% +1.0% 638.22 KB 647.53 KB 150.17 KB 151.75 KB NODE_DEV
react-dom.production.min.js 🔺+2.2% 🔺+1.7% 95.03 KB 97.17 KB 30.48 KB 31 KB NODE_PROD
react-dom-test-utils.development.js +0.3% +0.2% 44.84 KB 44.97 KB 12.15 KB 12.17 KB UMD_DEV
react-dom-test-utils.production.min.js 🔺+0.2% 🔺+0.2% 10.48 KB 10.5 KB 3.86 KB 3.86 KB UMD_PROD
react-dom-test-utils.development.js +0.3% +0.2% 44.56 KB 44.7 KB 12.09 KB 12.11 KB NODE_DEV
react-dom-test-utils.production.min.js 🔺+0.2% 🔺+0.2% 10.28 KB 10.3 KB 3.79 KB 3.79 KB NODE_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.02 KB 60.02 KB 15.73 KB 15.73 KB UMD_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 0.0% 11.04 KB 11.04 KB 3.8 KB 3.8 KB UMD_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 59.69 KB 59.69 KB 15.6 KB 15.6 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 0.0% 10.86 KB 10.86 KB 3.73 KB 3.73 KB NODE_PROD
react-dom-server.browser.development.js +0.5% +0.4% 103.7 KB 104.21 KB 27.7 KB 27.82 KB UMD_DEV
react-dom-server.browser.production.min.js 🔺+0.6% 🔺+0.6% 15.32 KB 15.42 KB 5.84 KB 5.88 KB UMD_PROD
react-dom-server.browser.development.js +0.5% +0.4% 99.83 KB 100.34 KB 26.73 KB 26.84 KB NODE_DEV
react-dom-server.browser.production.min.js 🔺+0.6% 🔺+0.7% 15.22 KB 15.32 KB 5.78 KB 5.82 KB NODE_PROD
react-dom-server.node.development.js +0.5% +0.4% 101.76 KB 102.27 KB 27.26 KB 27.37 KB NODE_DEV
react-dom-server.node.production.min.js 🔺+0.6% 🔺+0.7% 16.03 KB 16.13 KB 6.09 KB 6.13 KB NODE_PROD
ReactDOM-dev.js +1.5% +1.1% 644.97 KB 654.87 KB 148.23 KB 149.88 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+2.8% 🔺+1.7% 277.57 KB 285.47 KB 52.09 KB 53 KB FB_WWW_PROD
ReactTestUtils-dev.js +0.3% +0.2% 41.65 KB 41.8 KB 11.14 KB 11.17 KB FB_WWW_DEV
ReactDOMUnstableNativeDependencies-prod.js 0.0% -0.0% 26.16 KB 26.16 KB 5.28 KB 5.28 KB FB_WWW_PROD
ReactDOMServer-dev.js +0.5% +0.4% 100.97 KB 101.48 KB 26.42 KB 26.52 KB FB_WWW_DEV
ReactDOMServer-prod.js 🔺+0.5% 🔺+0.5% 33.1 KB 33.27 KB 7.99 KB 8.03 KB FB_WWW_PROD
react-dom.profiling.min.js +2.2% +1.8% 96.24 KB 98.37 KB 30.86 KB 31.41 KB NODE_PROFILING
ReactDOM-profiling.js +2.8% +1.7% 279.99 KB 287.91 KB 52.73 KB 53.65 KB FB_WWW_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +2.1% +1.6% 432.4 KB 441.69 KB 98.35 KB 99.9 KB UMD_DEV
react-art.production.min.js 🔺+2.5% 🔺+1.9% 84.48 KB 86.61 KB 26.2 KB 26.7 KB UMD_PROD
react-art.development.js +2.6% +1.9% 364.92 KB 374.23 KB 81.29 KB 82.81 KB NODE_DEV
react-art.production.min.js 🔺+4.3% 🔺+3.4% 49.47 KB 51.59 KB 15.56 KB 16.08 KB NODE_PROD
ReactART-dev.js +2.8% +2.2% 354.75 KB 364.65 KB 75.99 KB 77.62 KB FB_WWW_DEV
ReactART-prod.js 🔺+5.1% 🔺+3.4% 152.03 KB 159.77 KB 26.18 KB 27.08 KB FB_WWW_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +2.8% +2.0% 360.81 KB 370.88 KB 79.7 KB 81.32 KB UMD_DEV
react-test-renderer.production.min.js 🔺+5.0% 🔺+3.4% 48.35 KB 50.74 KB 15 KB 15.52 KB UMD_PROD
react-test-renderer.development.js +2.8% +2.1% 356.94 KB 367.01 KB 78.73 KB 80.36 KB NODE_DEV
react-test-renderer.production.min.js 🔺+5.0% 🔺+3.4% 48.05 KB 50.45 KB 14.84 KB 15.35 KB NODE_PROD
react-test-renderer-shallow.development.js +2.1% +1.8% 23.95 KB 24.46 KB 6.51 KB 6.63 KB UMD_DEV
react-test-renderer-shallow.development.js +2.7% +2.2% 19.14 KB 19.65 KB 5.33 KB 5.45 KB NODE_DEV
react-test-renderer-shallow.production.min.js 🔺+1.3% 🔺+1.5% 7.78 KB 7.88 KB 2.57 KB 2.61 KB NODE_PROD
ReactTestRenderer-dev.js +2.9% +2.2% 361.41 KB 372.03 KB 77.5 KB 79.2 KB FB_WWW_DEV
ReactShallowRenderer-dev.js +2.9% +2.5% 17.46 KB 17.97 KB 4.6 KB 4.72 KB FB_WWW_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +2.7% +2.0% 345.93 KB 355.25 KB 75.34 KB 76.86 KB NODE_DEV
react-reconciler.production.min.js 🔺+4.6% 🔺+3.4% 46.94 KB 49.08 KB 14.24 KB 14.71 KB NODE_PROD
react-reconciler-persistent.development.js +2.7% +2.0% 344.55 KB 353.86 KB 74.79 KB 76.3 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+4.6% 🔺+3.4% 46.95 KB 49.09 KB 14.24 KB 14.72 KB NODE_PROD
react-reconciler-reflection.development.js +4.0% +3.0% 14.32 KB 14.9 KB 4.5 KB 4.63 KB NODE_DEV
react-reconciler-reflection.production.min.js 0.0% 🔺+0.1% 2.55 KB 2.55 KB 1.13 KB 1.13 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +2.1% +1.6% 480.5 KB 490.43 KB 106.58 KB 108.27 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+3.6% 🔺+2.4% 211.71 KB 219.37 KB 37.23 KB 38.14 KB RN_FB_PROD
ReactNativeRenderer-dev.js +2.1% +1.6% 480.23 KB 490.17 KB 106.52 KB 108.21 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 🔺+3.8% 🔺+2.5% 201.76 KB 209.42 KB 35.66 KB 36.56 KB RN_OSS_PROD
ReactFabric-dev.js +2.1% +1.6% 470.71 KB 480.64 KB 104.14 KB 105.84 KB RN_FB_DEV
ReactFabric-prod.js 🔺+4.0% 🔺+2.6% 194.03 KB 201.7 KB 34.14 KB 35.03 KB RN_FB_PROD
ReactFabric-dev.js +2.1% +1.6% 470.74 KB 480.68 KB 104.15 KB 105.86 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+4.0% 🔺+2.6% 194.06 KB 201.74 KB 34.15 KB 35.04 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +3.7% +2.4% 205.18 KB 212.84 KB 36.31 KB 37.2 KB RN_OSS_PROFILING
ReactFabric-profiling.js +3.9% +2.5% 197.03 KB 204.71 KB 34.71 KB 35.59 KB RN_OSS_PROFILING
ReactNativeRenderer-profiling.js +3.6% +2.4% 215.1 KB 222.78 KB 37.88 KB 38.8 KB RN_FB_PROFILING
ReactFabric-profiling.js +3.9% +2.5% 196.99 KB 204.67 KB 34.69 KB 35.57 KB RN_FB_PROFILING

Generated by 🚫 dangerJS

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Aug 14, 2018

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.

@acdlite acdlite mentioned this pull request Aug 14, 2018
(current.type === elementType ||
(elementType !== null &&
elementType !== undefined &&
current.type === elementType._reactResult))
Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 14, 2018

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);
Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 14, 2018

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.

Copy link
Member Author

@acdlite acdlite Aug 14, 2018

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.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Aug 15, 2018

Don't forget to deal with defaultProps.

@acdlite
Copy link
Member Author

acdlite commented Aug 15, 2018

@sebmarkbage Updated

Component,
renderExpirationTime,
);
workInProgress.pendingProps = workInProgress.memoizedProps = props;
Copy link
Member Author

@acdlite acdlite Aug 15, 2018

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;
Copy link
Member Author

@acdlite acdlite Aug 15, 2018

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;
Copy link
Member

@gaearon gaearon Aug 15, 2018

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

Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 15, 2018

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.

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?

Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 15, 2018

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?

Copy link
Member

@gaearon gaearon Aug 15, 2018

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.

@Lucifier129
Copy link

Lucifier129 commented Aug 15, 2018

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.
@acdlite acdlite force-pushed the promiseaselementtype branch 3 times, most recently from 6c75678 to 2911577 Compare Aug 15, 2018
export function getLazyComponentTypeIfResolved<T>(
thenable: Thenable<T>,
): T | null {
return thenable._reactStatus === Resolved ? thenable._reactResult : null;
Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 15, 2018

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.

Copy link
Member Author

@acdlite acdlite Aug 15, 2018

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.

Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 15, 2018

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.

Copy link
Member Author

@acdlite acdlite Aug 15, 2018

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';
Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 15, 2018

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.

Copy link
Member Author

@acdlite acdlite Aug 15, 2018

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.

Copy link
Member Author

@acdlite acdlite Aug 15, 2018

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' ||
Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 15, 2018

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.

Copy link
Member Author

@acdlite acdlite Aug 15, 2018

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.

Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 15, 2018

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;
Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 15, 2018

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.

@acdlite acdlite requested a review from sebmarkbage Aug 15, 2018
@acdlite
Copy link
Member Author

acdlite commented Aug 15, 2018

Sorry I was testing the ability to re-request review :D Didn't know you could do that

screen shot 2018-08-15 at 2 55 56 pm

);
}
case ClassComponent: {
const Component = workInProgress.type;
Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 15, 2018

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.

Copy link
Member Author

@acdlite acdlite Aug 15, 2018

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

Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 15, 2018

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.

Copy link
Member Author

@acdlite acdlite Aug 15, 2018

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

Copy link
Member Author

@acdlite acdlite Aug 15, 2018

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'
Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 15, 2018

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) {
Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 15, 2018

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;
Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 15, 2018

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.

Copy link
Member Author

@acdlite acdlite Aug 15, 2018

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 &&
Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 15, 2018

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.

);
}
}
workInProgress.pendingProps = props;
Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Member Author

@acdlite acdlite Aug 16, 2018

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.

Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 16, 2018

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.

Copy link
Member Author

@acdlite acdlite Aug 16, 2018

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

Copy link
Member Author

@acdlite acdlite Aug 16, 2018

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.

@acdlite acdlite requested a review from sebmarkbage Aug 16, 2018
import {
refineResolvedThenable,
getResultFromResolvedThenable,
} from 'react-reconciler/src/ReactFiberLazyComponent';
Copy link
Member Author

@acdlite acdlite Aug 16, 2018

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.

Copy link
Member

@gaearon gaearon Aug 16, 2018

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(
Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 16, 2018

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(
Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 16, 2018

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;
Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 16, 2018

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.

Copy link

@arianon arianon Aug 16, 2018

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?

Copy link
Member

@gaearon gaearon Aug 16, 2018

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.

Copy link

@arianon arianon Aug 16, 2018

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.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Mostly nits left. Just make sure to fix that getComponentName thing.

@acdlite acdlite merged commit 5031ebf into facebook:master Aug 16, 2018
1 check was pending
@gaearon
Copy link
Member

gaearon commented Aug 16, 2018

@acdlite Will you prepare a DevTools PR that forks the type of work list?

@acdlite
Copy link
Member Author

acdlite commented Aug 17, 2018

@gaearon Uh sure but you'll have to give me more details :D I have zero clue how DevTools works.

@gaearon
Copy link
Member

gaearon commented Aug 17, 2018

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;
Copy link
Member

@gaearon gaearon Aug 20, 2018

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.

Copy link
Member

@gaearon gaearon Aug 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix in #13441

@gaearon
Copy link
Member

gaearon commented Aug 20, 2018

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);
Copy link
Member

@gaearon gaearon Aug 20, 2018

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?

@gaearon
Copy link
Member

gaearon commented Aug 20, 2018

DevTools support: facebook/react-devtools#1103

TejasQ pushed a commit to TejasQ/react that referenced this issue Aug 26, 2018
* 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
NMinhNguyen referenced this issue in enzymejs/react-shallow-renderer Jan 29, 2020
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants