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.

Sign up for GitHub

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

Jump to bottom

[Fiber] Sync mount and unmount #8634

Merged
merged 6 commits into from
Dec 29, 2016
Merged

[Fiber] Sync mount and unmount #8634

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ffbb86b
Wrap top-level mount and unmount in syncUpdates
acdlite Dec 29, 2016
3926a35
If an error is thrown and there's no nextUnitOfWork, it's fatal error
acdlite Dec 29, 2016
8090cf5
Allow performWork to be called recursively
acdlite Dec 29, 2016
21f6d41
syncUpdates prevents Synchronous priority from being downgraded to Task
acdlite Dec 29, 2016
ce8c978
Disallow forced sync updates during begin phase
acdlite Dec 29, 2016
79f01b2
prepareForCommit returns info object to be passed resetAfterCommit
acdlite Dec 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions scripts/fiber/tests-passing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1207,6 +1207,7 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js
* can opt-in to deferred/animation scheduling inside componentDidMount/Update
* performs Task work even after time runs out
* does not perform animation work after time runs out
* can force synchronous updates with syncUpdates, even inside batchedUpdates

src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js
* can update child nodes of a host instance
Expand Down Expand Up @@ -1576,6 +1577,7 @@ src/renderers/shared/shared/__tests__/ReactUpdates-test.js
* unstable_batchedUpdates should return value from a callback
* unmounts and remounts a root in the same batch
* handles reentrant mounting in synchronous mode
* mounts and unmounts are sync even in a batch

src/renderers/shared/shared/__tests__/refs-destruction-test.js
* should remove refs when destroying the parent
Expand Down
41 changes: 26 additions & 15 deletions src/renderers/dom/fiber/ReactDOMFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,10 @@ type HostContextDev = {
};
type HostContextProd = string;
type HostContext = HostContextDev | HostContextProd;

let eventsEnabled : ?boolean = null;
let selectionInformation : ?mixed = null;
type CommitInfo = {
eventsEnabled: boolean,
selectionInformation: mixed,
};

var ELEMENT_NODE_TYPE = 1;
var DOC_NODE_TYPE = 9;
Expand Down Expand Up @@ -137,17 +138,18 @@ var DOMRenderer = ReactFiberReconciler({
return getChildNamespace(parentNamespace, type);
},

prepareForCommit() : void {
eventsEnabled = ReactBrowserEventEmitter.isEnabled();
prepareForCommit() : CommitInfo {
const eventsEnabled = ReactBrowserEventEmitter.isEnabled();
ReactBrowserEventEmitter.setEnabled(false);
selectionInformation = ReactInputSelection.getSelectionInformation();
return {
Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW this is how @spicyj wrote it originally but @sebmarkbage wanted to avoid the allocation.

Copy link
Collaborator Author

@acdlite acdlite Dec 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I see. I don't see a way to avoid it given that a sync update may occur inside componentWillUnmount. Unless we disallow sync updates there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That actually might be a good idea... Interleaving commit phases could cause trouble.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since nested interleaved commits are broken anyway, can we revert this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops yes I meant to do that, forgot!

eventsEnabled,
selectionInformation: ReactInputSelection.getSelectionInformation(),
};
},

resetAfterCommit() : void {
ReactInputSelection.restoreSelection(selectionInformation);
selectionInformation = null;
ReactBrowserEventEmitter.setEnabled(eventsEnabled);
eventsEnabled = null;
resetAfterCommit(commitInfo : CommitInfo) : void {
ReactInputSelection.restoreSelection(commitInfo.selectionInformation);
ReactBrowserEventEmitter.setEnabled(commitInfo.eventsEnabled);
},

createInstance(
Expand Down Expand Up @@ -322,9 +324,15 @@ function renderSubtreeIntoContainer(parentComponent : ?ReactComponent<any, any,
while (container.lastChild) {
container.removeChild(container.lastChild);
}
root = container._reactRootContainer = DOMRenderer.createContainer(container);
const newRoot = DOMRenderer.createContainer(container);
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that syncUpdates immediately executes the callback you pass it, why is the new newRoot variable necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To satisfy Flow, which complains otherwise. I'm guessing because it doesn't know that syncUpdates executes its callback synchronously.

root = container._reactRootContainer = newRoot;
// Initial mount is always sync, even if we're in a batch.
DOMRenderer.syncUpdates(() => {
DOMRenderer.updateContainer(children, newRoot, parentComponent, callback);
});
} else {
DOMRenderer.updateContainer(children, root, parentComponent, callback);
}
DOMRenderer.updateContainer(children, root, parentComponent, callback);
return DOMRenderer.getPublicRootInstance(root);
}

Expand All @@ -346,8 +354,11 @@ var ReactDOM = {
unmountComponentAtNode(container : DOMContainerElement) {
warnAboutUnstableUse();
if (container._reactRootContainer) {
return renderSubtreeIntoContainer(null, null, container, () => {
container._reactRootContainer = null;
// Unmount is always sync, even if we're in a batch.
return DOMRenderer.syncUpdates(() => {
return renderSubtreeIntoContainer(null, null, container, () => {
container._reactRootContainer = null;
});
});
}
},
Expand Down
19 changes: 17 additions & 2 deletions src/renderers/shared/__tests__/ReactPerf-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ describe('ReactPerf', () => {
var ReactDOM;
var ReactPerf;
var ReactTestUtils;
var ReactDOMFeatureFlags;
var emptyFunction;

var App;
Expand All @@ -38,6 +39,7 @@ describe('ReactPerf', () => {
ReactDOM = require('ReactDOM');
ReactPerf = require('ReactPerf');
ReactTestUtils = require('ReactTestUtils');
ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
emptyFunction = require('emptyFunction');

App = class extends React.Component {
Expand Down Expand Up @@ -614,7 +616,7 @@ describe('ReactPerf', () => {
}
}
class EvilPortal extends React.Component {
componentWillMount() {
componentDidMount() {
var portalContainer = document.createElement('div');
ReactDOM.render(<Evil />, portalContainer);
}
Expand Down Expand Up @@ -645,6 +647,10 @@ describe('ReactPerf', () => {
var container = document.createElement('div');
var thrownErr = new Error('Muhaha!');

if (ReactDOMFeatureFlags.useFiber) {
spyOn(console, 'error');
}

class Evil extends React.Component {
componentWillMount() {
throw thrownErr;
Expand Down Expand Up @@ -679,6 +685,15 @@ describe('ReactPerf', () => {
}
ReactDOM.unmountComponentAtNode(container);
ReactPerf.stop();

if (ReactDOMFeatureFlags.useFiber) {
// A sync `render` inside cWM will print a warning. That should be the
// only warning.
expect(console.error.calls.count()).toEqual(1);
expect(console.error.calls.argsFor(0)[0]).toMatch(
/Render methods should be a pure function of props and state/
);
}
});

it('should not print errant warnings if portal throws in componentDidMount()', () => {
Expand All @@ -694,7 +709,7 @@ describe('ReactPerf', () => {
}
}
class EvilPortal extends React.Component {
componentWillMount() {
componentDidMount() {
var portalContainer = document.createElement('div');
ReactDOM.render(<Evil />, portalContainer);
}
Expand Down
4 changes: 2 additions & 2 deletions src/renderers/shared/fiber/ReactFiberBeginWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ if (__DEV__) {
var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber');
}

module.exports = function<T, P, I, TI, C, CX>(
config : HostConfig<T, P, I, TI, C, CX>,
module.exports = function<T, P, I, TI, C, CX, CI>(
config : HostConfig<T, P, I, TI, C, CX, CI>,
hostContext : HostContext<C, CX>,
scheduleUpdate : (fiber : Fiber, priorityLevel : PriorityLevel) => void,
getPriorityContext : () => PriorityLevel,
Expand Down
4 changes: 2 additions & 2 deletions src/renderers/shared/fiber/ReactFiberCommitWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ var {
ContentReset,
} = require('ReactTypeOfSideEffect');

module.exports = function<T, P, I, TI, C, CX>(
config : HostConfig<T, P, I, TI, C, CX>,
module.exports = function<T, P, I, TI, C, CX, CI>(
config : HostConfig<T, P, I, TI, C, CX, CI>,
hostContext : HostContext<C, CX>,
captureError : (failedFiber : Fiber, error: Error) => ?Fiber
) {
Expand Down
4 changes: 2 additions & 2 deletions src/renderers/shared/fiber/ReactFiberCompleteWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ if (__DEV__) {
var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber');
}

module.exports = function<T, P, I, TI, C, CX>(
config : HostConfig<T, P, I, TI, C, CX>,
module.exports = function<T, P, I, TI, C, CX, CI>(
config : HostConfig<T, P, I, TI, C, CX, CI>,
hostContext : HostContext<C, CX>,
) {
const {
Expand Down
4 changes: 2 additions & 2 deletions src/renderers/shared/fiber/ReactFiberHostContext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ export type HostContext<C, CX> = {
resetHostContainer() : void,
};

module.exports = function<T, P, I, TI, C, CX>(
config : HostConfig<T, P, I, TI, C, CX>
module.exports = function<T, P, I, TI, C, CX, CI>(
config : HostConfig<T, P, I, TI, C, CX, CI>
) : HostContext<C, CX> {
const {
getChildHostContext,
Expand Down
8 changes: 4 additions & 4 deletions src/renderers/shared/fiber/ReactFiberReconciler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export type Deadline = {

type OpaqueNode = Fiber;

export type HostConfig<T, P, I, TI, C, CX> = {
export type HostConfig<T, P, I, TI, C, CX, CI> = {

getRootHostContext(rootContainerInstance : C) : CX,
getChildHostContext(parentHostContext : CX, type : T) : CX,
Expand All @@ -69,8 +69,8 @@ export type HostConfig<T, P, I, TI, C, CX> = {
scheduleAnimationCallback(callback : () => void) : void,
scheduleDeferredCallback(callback : (deadline : Deadline) => void) : void,

prepareForCommit() : void,
resetAfterCommit() : void,
prepareForCommit() : CI,
resetAfterCommit(commitInfo : CI) : void,

useSyncScheduling ?: boolean,
};
Expand Down Expand Up @@ -100,7 +100,7 @@ getContextForSubtree._injectFiber(function(fiber : Fiber) {
parentContext;
});

module.exports = function<T, P, I, TI, C, CX>(config : HostConfig<T, P, I, TI, C, CX>) : Reconciler<C, I, TI> {
module.exports = function<T, P, I, TI, C, CX, CI>(config : HostConfig<T, P, I, TI, C, CX, CI>) : Reconciler<C, I, TI> {

var {
scheduleUpdate,
Expand Down
Loading