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 1 commit
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
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"serve": "webpack-dev-server --host 0.0.0.0 --hot --inline --history-api-fallback",
"start": "npm-run-all --parallel karma:dev serve",
"karma:once": "karma start --single-run",
"karma:dev": "karma start --browsers Chrome",
"karma:dev": "karma start --browsers Chromium",
"babel": "babel src -d lib",
"build": "npm-run-all clean babel",
"lint": "eslint '*.js' '{src,test}/**/*.js*'",
Expand Down
53 changes: 26 additions & 27 deletions test/Content.spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,20 @@
import React from 'react';
import ReactDOM, { unmountComponentAtNode } from 'react-dom';
import ReactTestUtils from 'react-dom/test-utils';
import { expect } from 'chai';
import sinon from 'sinon/pkg/sinon';
import Content from '../src/Content';

describe('The Content component', () => {
let div;

afterEach(() => {
if (div) {
div.remove();
div = null;
}
});

it('should render children', () => {
const content = ReactTestUtils.renderIntoDocument(
<Content
Expand All @@ -16,8 +26,8 @@ describe('The Content component', () => {
</Content>
);

const div = ReactTestUtils.findRenderedDOMComponentWithTag(content, 'div');
expect(div.className).to.equal('test-class-1');
const elem = ReactTestUtils.findRenderedDOMComponentWithTag(content, 'div');
expect(elem.className).to.equal('test-class-1');
});

it('should call contentDidMount on initial render', () => {
Expand Down Expand Up @@ -60,39 +70,28 @@ describe('The Content component', () => {
});

it('should call contentWillUnmount before unmounting', done => {
div = document.body.appendChild(document.createElement('div'));
const didMount = sinon.spy();
const didUpdate = sinon.spy();
const willUnmount = sinon.spy();

class Parent extends React.Component {
state = {
isClosed: false
};

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

const wrapper = ReactTestUtils.renderIntoDocument(<Parent />);
ReactDOM.render(<Parent />, div);

expect(didMount.callCount).to.equal(1);
expect(didUpdate.callCount).to.equal(0);
wrapper.setState({ isClosed: true }, () => {
expect(willUnmount.callCount).to.equal(1);
done();
});
unmountComponentAtNode(div);
expect(willUnmount.callCount).to.equal(1);
done();
});

it('should error if null is passed in contentWillUnmount', () => {
Expand Down
38 changes: 11 additions & 27 deletions test/Frame.spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React from 'react';
import PropTypes from 'prop-types';
import ReactDOM from 'react-dom';
import ReactDOM, { unmountComponentAtNode } from 'react-dom';
import ReactTestUtils from 'react-dom/test-utils';
import { expect } from 'chai';
import sinon from 'sinon/pkg/sinon';
Expand Down Expand Up @@ -317,36 +317,20 @@ describe('The Frame Component', () => {
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?


class Parent extends React.Component {
constructor(props) {
super(props);
this.state = {
isClosed: false
};
}

render() {
if (!this.state.isClosed) {
return (
<Frame
contentWillUnmount={() => {
willUnmount();
}}
>
<div className="test-class-1" />
</Frame>
);
}
return null;
}
}
const Thing = () => (
<Frame contentWillUnmount={willUnmount}>
<div className="test-class-1" />
</Frame>
);

const wrapper = ReactDOM.render(<Parent />, div);
ReactDOM.render(<Thing />, div);

wrapper.setState({ isClosed: true }, () => {
// TODO: act() doesn'tt seem to solve this something weird???
setTimeout(() => {
Copy link
Owner

@ryanseddon ryanseddon Jun 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@Reflex-Gravity not sure why act() didn't work but this might give you a clue as to what's happening here, srcdoc is async and act doesn't seem to wait long enough for the iframe to actually mount but doing the old setTimeout next tick trick does...

unmountComponentAtNode(div);
expect(willUnmount.callCount).to.equal(1, 'expected 1 willUnmount');
done();
});
}, 100);
});

it('should error when null is passed in contentWillUnmount', () => {
Expand Down