Skip to content

Added contentWillUnmount prop to call before the Contents unmounts. #185

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Added contentWillUnmount prop to call before the Contents unmounts. #185

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
60a04d6
Added contentWillUnmount to call before unmounting
Apr 30, 2021
d7bceae
added tests for contentWillUnmount
Apr 30, 2021
08a9e14
removed contentWillUnmount as required prop
Apr 30, 2021
b15f3c8
Added tests for passing null in contentWillUnmount
Apr 30, 2021
7d4418a
Merge branch 'ryanseddon:master' into willUnMount-feat
Reflex-Gravity May 29, 2021
b5099c7
Updated tests and Proptype for contentWillUnmount
May 29, 2021
e1fa1b4
Updated tests for Content, removed defaultProps in content
Jun 1, 2021
9bd4b98
Updated test for Frame
Jun 1, 2021
58d01e4
Merge branch 'master' into willUnMount-feat
Reflex-Gravity Jun 11, 2021
84443a4
Merge branch 'master' into willUnMount-feat
ryanseddon Jun 19, 2021
208e7a5
pass unmount tests
ryanseddon Jun 19, 2021
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
13 changes: 12 additions & 1 deletion src/Content.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@ export default class Content extends Component {
static propTypes = {
children: PropTypes.element.isRequired,
contentDidMount: PropTypes.func.isRequired,
contentDidUpdate: PropTypes.func.isRequired
contentDidUpdate: PropTypes.func.isRequired,
contentWillUnmount: PropTypes.func
};

static defaultProps = {
contentWillUnmount: null
};

componentDidMount() {
Expand All @@ -16,6 +21,12 @@ export default class Content extends Component {
this.props.contentDidUpdate();
}

componentWillUnmount() {
if (typeof this.props.contentWillUnmount === 'function') {
this.props.contentWillUnmount();
}
}

render() {
return Children.only(this.props.children);
}
Expand Down
5 changes: 5 additions & 0 deletions src/Frame.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export default class Frame extends Component {
mountTarget: PropTypes.string,
contentDidMount: PropTypes.func,
contentDidUpdate: PropTypes.func,
contentWillUnmount: PropTypes.func,
children: PropTypes.oneOfType([
PropTypes.element,
PropTypes.arrayOf(PropTypes.element)
Expand All @@ -29,6 +30,7 @@ export default class Frame extends Component {
mountTarget: undefined,
contentDidMount: () => {},
contentDidUpdate: () => {},
contentWillUnmount: () => {},
initialContent:
'<!DOCTYPE html><html><head></head><body><div class="frame-root"></div></body></html>'
};
Expand Down Expand Up @@ -89,12 +91,14 @@ export default class Frame extends Component {

const contentDidMount = this.props.contentDidMount;
const contentDidUpdate = this.props.contentDidUpdate;
const contentWillUnmount = this.props.contentWillUnmount;

const win = doc.defaultView || doc.parentView;
const contents = (
<Content
contentDidMount={contentDidMount}
contentDidUpdate={contentDidUpdate}
contentWillUnmount={contentWillUnmount}
>
<FrameContextProvider value={{ document: doc, window: win }}>
<div className="frame-content">{this.props.children}</div>
Expand All @@ -121,6 +125,7 @@ export default class Frame extends Component {
delete props.mountTarget;
delete props.contentDidMount;
delete props.contentDidUpdate;
delete props.contentWillUnmount;
return (
<iframe {...props} ref={this.setRef} onLoad={this.handleLoad}>
{this.state.iframeLoaded && this.renderFrameContents()}
Expand Down
78 changes: 73 additions & 5 deletions test/Content.spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react';
import React, { useEffect } from 'react';
import ReactTestUtils from 'react-dom/test-utils';
import { expect } from 'chai';
import sinon from 'sinon/pkg/sinon';
Expand All @@ -7,7 +7,11 @@ import Content from '../src/Content';
describe('The Content component', () => {
it('should render children', () => {
const content = ReactTestUtils.renderIntoDocument(
<Content contentDidMount={() => null} contentDidUpdate={() => null}>
<Content
contentDidMount={() => null}
contentDidUpdate={() => null}
contentWillUnmount={() => null}
>
<div className="test-class-1" />
</Content>
);
Expand All @@ -19,9 +23,14 @@ describe('The Content component', () => {
it('should call contentDidMount on initial render', () => {
const didMount = sinon.spy();
const didUpdate = sinon.spy();
const willUnmount = sinon.spy();

ReactTestUtils.renderIntoDocument(
<Content contentDidMount={didMount} contentDidUpdate={didUpdate}>
<Content
contentDidMount={didMount}
contentDidUpdate={didUpdate}
contentWillUnmount={willUnmount}
>
<div className="test-class-1" />
</Content>
);
Expand All @@ -30,12 +39,17 @@ describe('The Content component', () => {
expect(didUpdate.callCount).to.equal(0);
});

it('should call contentDidUpdate on subsequent updates', (done) => {
it('should call contentDidUpdate on subsequent updates', done => {
const didMount = sinon.spy();
const didUpdate = sinon.spy();
const willUnmount = sinon.spy();

const content = ReactTestUtils.renderIntoDocument(
<Content contentDidMount={didMount} contentDidUpdate={didUpdate}>
<Content
contentDidMount={didMount}
contentDidUpdate={didUpdate}
contentWillUnmount={willUnmount}
>
<div className="test-class-1" />
</Content>
);
Expand All @@ -54,4 +68,58 @@ describe('The Content component', () => {
});
});
});

it('should call contentWillUnmount before unmounting', () => {
const didMount = sinon.spy();
const didUpdate = sinon.spy();
const willUnmount = sinon.spy();

function Parent() {
const [isClosed, setClosed] = React.useState(false);

useEffect(() => {
setTimeout(() => {
setClosed(true);
}, 1500);
}, []);

if (!isClosed) {
return (
<Content
contentDidMount={didMount}
contentDidUpdate={didUpdate}
contentWillUnmount={willUnmount}
>
<div className="test-class-1" />
</Content>
);
}
return null;
}

ReactTestUtils.renderIntoDocument(<Parent />);

expect(didMount.callCount).to.equal(1);
expect(didUpdate.callCount).to.equal(0);
});

it('should not error if null is passed in contentWillUnmount', () => {
ReactTestUtils.renderIntoDocument(
<Content
contentDidMount={() => null}
contentDidUpdate={() => null}
contentWillUnmount={null}
>
<div className="test-class-1" />
</Content>
);
});

it('should not error if contentWillUnmount is not passed as a prop', () => {
ReactTestUtils.renderIntoDocument(
<Content contentDidMount={() => null} contentDidUpdate={() => null}>
<div className="test-class-1" />
</Content>
);
});
});
61 changes: 59 additions & 2 deletions test/Frame.spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react';
import React, { useEffect } from 'react';
import PropTypes from 'prop-types';
import ReactDOM from 'react-dom';
import ReactTestUtils from 'react-dom/test-utils';
Expand Down Expand Up @@ -313,6 +313,63 @@ describe('The Frame Component', () => {
);
});

it('should call contentWillUnmount on unmount', done => {
div = document.body.appendChild(document.createElement('div'));
const willUnmount = sinon.spy();
Copy link
Owner

Choose a reason for hiding this comment

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

I see this is called but this test doesn't check the callCount either?

Copy link
Author

Choose a reason for hiding this comment

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

So, I just need to check the callCount to be 1 ryt?

Copy link
Owner

Choose a reason for hiding this comment

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

yep

Copy link
Author

@Reflex-Gravity Reflex-Gravity Jun 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I have updated this test. But, the test is failing. The contentWillUnmount isn't getting called, maybe because the unmounting of the component in the test isn't happening properly.
I don't exactly get the reason for this. The implementation of the feature seems to be correct and working. But the way I wrote the test is what I guess is wrong. Can you help me here?

Copy link
Owner

Choose a reason for hiding this comment

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

Ok so i think what we're doing here is actually the wrong thing, the issue is about having a callback when the <Frame> component unmounts and not when the contents within the iframe unmount.

So I think what we need is a bit simpler instead of passing the contentWillUnmount prop down through to <Content> we just need to call it inside <Frame>'s componentWillUnmount callback and we should probably rename it to frameWillUnmount to differentiate it from the other content lifecycle events.

Then you can just test it like this:

  it('should call contentWillUnmount on unmount', done => {
    div = document.body.appendChild(document.createElement('div'));
    const willUnmount = sinon.spy();
    const frame = ReactDOM.render(
      <Frame frameWillUnmount={willUnmount} />,
      div
    );
    ReactDOM.unmountComponentAtNode(ReactDOM.findDOMNode(frame).parentNode);
    expect(willUnmount.callCount).to.equal(1, 'expected 1 willUnmount');
    done();
  });

Copy link
Author

@Reflex-Gravity Reflex-Gravity Jun 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Okay. But still contentWillUnmount should be called as Content gets unmounted too.
I tried it in an example, the contentWillUnmount was called when I unmounted the Frame. The test however doesn't trigger the callback.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah ok having though on this more I think what you have is useful in two instances of changing the iframe contents as well as unmounting the entire Frame too.

I honestly have no idea why this isn't firing in test env, if you run npm run karma:dev that will allow you to debug the tests within Chrome dev tools.

Copy link
Author

Choose a reason for hiding this comment

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

Tried with npm run karma:dev same issue. How do I proceed? Skip this test?


function Parent() {
const [isClosed, setClosed] = React.useState(false);

useEffect(() => {
setTimeout(() => {
setClosed(true);
}, 1500);
}, []);

if (!isClosed) {
return (
<Frame
contentWillUnmount={() => {
willUnmount();
done();
}}
/>
);
}
return null;
}
ReactDOM.render(<Parent />, div);
});

it('should not error when null is passed in contentWillUnmount', () => {
div = document.body.appendChild(document.createElement('div'));
ReactDOM.render(<Frame contentWillUnmount={null} />, div);
Copy link
Owner

Choose a reason for hiding this comment

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

Also these tests should assert something?

});

it('should not error when contentWillUnmount is not passed', done => {
div = document.body.appendChild(document.createElement('div'));
const didUpdate = sinon.spy();
const didMount = sinon.spy();
const frame = ReactDOM.render(
<Frame
contentDidUpdate={didUpdate}
contentDidMount={() => {
didMount();
frame.setState({ foo: 'bar' }, () => {
expect(didMount.callCount).to.equal(1, 'expected 1 didMount');
expect(didUpdate.callCount).to.equal(1, 'expected 1 didUpdate');
frame.setState({ foo: 'gah' }, () => {
expect(didMount.callCount).to.equal(1, 'expected 1 didMount');
expect(didUpdate.callCount).to.equal(2, 'expected 2 didUpdate');
done();
});
});
}}
/>,
div
);
Copy link
Owner

Choose a reason for hiding this comment

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

Same here nothing is asserted?

});

it('should return first child element of the `body` on call to `this.getMountTarget()` if `props.mountTarget` was not passed in', () => {
div = document.body.appendChild(document.createElement('div'));

Expand Down Expand Up @@ -424,7 +481,7 @@ describe('The Frame Component', () => {
contentDidMount={() => {
const iframes = ReactDOM.findDOMNode(div).querySelectorAll('iframe');

expect(iframes[0].contentDocument.body.children.length).to.equal(1);
expect(iframes[0].contentDocument.body.children.length).to.equal(1);
expect(iframes[0].contentDocument.body.children.length).to.equal(1);
done();
}}
Expand Down