-
Notifications
You must be signed in to change notification settings - Fork 49.8k
Renderers can queue commit effects for initial mount #8646
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -96,6 +96,20 @@ function validateContainer(container) { | |||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| function shouldAutoFocusHostComponent( | ||||||||||
| type : string, | ||||||||||
| props : Props, | ||||||||||
| ) : boolean { | ||||||||||
| switch (type) { | ||||||||||
| case 'button': | ||||||||||
| case 'input': | ||||||||||
| case 'select': | ||||||||||
| case 'textarea': | ||||||||||
| return !!(props : any).autoFocus; | ||||||||||
| } | ||||||||||
| return false; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| var DOMRenderer = ReactFiberReconciler({ | ||||||||||
|
|
||||||||||
| getRootHostContext(rootContainerInstance : Container) : HostContext { | ||||||||||
|
|
@@ -173,8 +187,9 @@ var DOMRenderer = ReactFiberReconciler({ | |||||||||
| type : string, | ||||||||||
| props : Props, | ||||||||||
| rootContainerInstance : Container, | ||||||||||
| ) : void { | ||||||||||
| ) : boolean { | ||||||||||
| setInitialProperties(domElement, type, props, rootContainerInstance); | ||||||||||
| return shouldAutoFocusHostComponent(type, props); | ||||||||||
| }, | ||||||||||
|
|
||||||||||
| prepareUpdate( | ||||||||||
|
|
@@ -197,6 +212,18 @@ var DOMRenderer = ReactFiberReconciler({ | |||||||||
| return true; | ||||||||||
| }, | ||||||||||
|
|
||||||||||
| commitMount( | ||||||||||
| domElement : Instance, | ||||||||||
| type : string, | ||||||||||
| newProps : Props, | ||||||||||
| rootContainerInstance : Container, | ||||||||||
| internalInstanceHandle : Object, | ||||||||||
| ) : void { | ||||||||||
| if (shouldAutoFocusHostComponent(type, newProps)) { | ||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this extra
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check is still necessary.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But #8685 added a special tag for refs so it shouldn't be necessary for it to call
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, I think you're right, but there's this: react/src/renderers/shared/fiber/ReactFiberCompleteWork.js Lines 240 to 243 in 3bc5595
I think this is actually not intentional. I'll follow up on it. 😄
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ^ @acdlite
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that was an oversight on my part
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Follow-up on #8779 |
||||||||||
| (domElement : any).focus(); | ||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can cast this to something more specific that has the focus method on it. Such as
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are no longer using the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aye, the PR summary noted that:
Seemed wrong to keep an abstraction around solely for compat with a browser version we no longer support. |
||||||||||
| } | ||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain why we need this extra method? Could we not unify this with
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In fact the code to handle this is already in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I attempted to explain my reasoning in my previous comment. Some of it is subjective.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. Let's go ahead and call that method
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The previous code was in If you are referring to moving it into
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah you're right, I misunderstood when |
||||||||||
| }, | ||||||||||
|
|
||||||||||
| commitUpdate( | ||||||||||
| domElement : Instance, | ||||||||||
| type : string, | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -40,6 +40,7 @@ module.exports = function<T, P, I, TI, C, CX>( | |||
| ) { | ||||
|
|
||||
| const { | ||||
| commitMount, | ||||
| commitUpdate, | ||||
| resetTextContent, | ||||
| commitTextUpdate, | ||||
|
|
@@ -434,6 +435,21 @@ module.exports = function<T, P, I, TI, C, CX>( | |||
| case HostComponent: { | ||||
| const instance : I = finishedWork.stateNode; | ||||
| attachRef(current, finishedWork, instance); | ||||
|
|
||||
| // Renderers may schedule work to be done after host components are mounted | ||||
| // (eg DOM renderer may schedule auto-focus for inputs and form controls). | ||||
| // These effects should only be committed when components are first mounted, | ||||
| // aka when there is no current/alternate. | ||||
| if ( | ||||
| !current && | ||||
| finishedWork.effectTag & Update | ||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh shoot, missed this. Don't need to check for Update effect because it was already checked by the time
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If that's the case, why does the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because it also gets called if there's a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, right. That makes sense. Ok! |
||||
| ) { | ||||
| const type = finishedWork.type; | ||||
| const props = finishedWork.memoizedProps; | ||||
| const rootContainerInstance = getRootHostContainer(); | ||||
| commitMount(instance, type, props, rootContainerInstance, finishedWork); | ||||
| } | ||||
|
|
||||
| return; | ||||
| } | ||||
| case HostText: { | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,10 +50,11 @@ export type HostConfig<T, P, I, TI, C, CX> = { | |
|
|
||
| createInstance(type : T, props : P, rootContainerInstance : C, hostContext : CX, internalInstanceHandle : OpaqueNode) : I, | ||
| appendInitialChild(parentInstance : I, child : I | TI) : void, | ||
| finalizeInitialChildren(parentInstance : I, type : T, props : P, rootContainerInstance : C) : void, | ||
| finalizeInitialChildren(parentInstance : I, type : T, props : P, rootContainerInstance : C) : boolean, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could consider returning an constant enum number tag here. Like other tags. It's not less efficient as long as it gets inlined and the constants make it a bit more readable. It's also more extensible if we want to add more tags here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True! What other sorts of work do you think |
||
|
|
||
| prepareUpdate(instance : I, type : T, oldProps : P, newProps : P, hostContext : CX) : boolean, | ||
| commitUpdate(instance : I, type : T, oldProps : P, newProps : P, rootContainerInstance : C, internalInstanceHandle : OpaqueNode) : void, | ||
| commitMount(instance : I, type : T, newProps : P, rootContainerInstance : C, internalInstanceHandle : OpaqueNode) : void, | ||
|
|
||
| shouldSetTextContent(props : P) : boolean, | ||
| resetTextContent(instance : I) : void, | ||
|
|
||
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 expand the
Propstype to include this as optional instead of relying onanyhere.