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

Adds Mount tracking #1730

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

Adds Mount tracking #1730

wants to merge 11 commits into from

Conversation

TzviPM
Copy link

@TzviPM TzviPM commented Aug 2, 2018

Closes #1689. Closes #437.

@TzviPM
Copy link
Author

TzviPM commented Aug 2, 2018

looks like one of the jobs in my build timed out before starting the tests: https://travis-ci.org/airbnb/enzyme/jobs/411307270 cc @ljharb

* @param {ReactWrapper|ShallowWrapper} wrapper
*/
export function trackMountedWrapper(wrapper) {
const { enableMountTracking } = get();

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

export function trackMountedWrapper(wrapper) {
const { enableMountTracking } = get();
if (enableMountTracking) {
mountedWrappers.push(wrapper);
Copy link
Member

Choose a reason for hiding this comment

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

what if it's already in there? this should probably use a Set

@ljharb ljharb added the semver: minor New stuff. label Aug 3, 2018

configure({
adapter: new Adapter(),
enableMountTracking: true,
Copy link
Member

Choose a reason for hiding this comment

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

it seems kind of weird to call this option "mount tracking" when it tracks both shallow and mount. maybe "enableSandbox", since sinon-sandbox is actually the behavior it's emulating?

@@ -15,4 +16,6 @@ module.exports = {
ReactWrapper,
configure,
EnzymeAdapter,
trackMountedWrapper,
Copy link
Member

Choose a reason for hiding this comment

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

why does this need to be re-exported?

Copy link
Author

Choose a reason for hiding this comment

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

for custom implementation of mount that extend the enzyme wrapper. We use this pattern often, for example, to encapsulate logic around application context.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it needs to be in the root export tho - consumers can deep-import whatever they need from inside build

@TzviPM
Copy link
Author

TzviPM commented Aug 9, 2018

@ljharb can you have another look when you get a chance?


it('should call trackWrapper', () => {
const spy = sinon.spy();
const originalTrackWrapper = wrapperSandbox.trackWrapper;
Copy link
Member

Choose a reason for hiding this comment

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

eesh, this is a really brittle test that will definitely break in babel 7 and when node ships native ESM support. is this necessary? can we instead test the actual behavior?


it('should call trackWrapper', () => {
const spy = sinon.spy();
const originalTrackWrapper = wrapperSandbox.trackWrapper;
Copy link
Member

Choose a reason for hiding this comment

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

same here

unmountAllWrappers,
} from 'enzyme/build/wrapperSandbox';

const originalConfig = get();
Copy link
Member

Choose a reason for hiding this comment

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

this should probably be populated before or beforeEach rather than at require time

it('does what i expect', () => {
const wrapper = new ReactWrapper(<p>foo</p>);
const spy = sinon.spy();
wrapper.unmount = spy;
Copy link
Member

Choose a reason for hiding this comment

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

const spy = sinon.spy(wrapper, 'unmount');

* until after you have restored those globals will lead to their stored
* identifiers being invalid.
*/
export function unmountAllWrappers() {
Copy link
Member

Choose a reason for hiding this comment

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

i kind of feel like if enableSandbox is false, this function should throw

@ljharb ljharb force-pushed the master branch 3 times, most recently from 43eb75e to 39e6b1f Compare November 3, 2022 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: minor New stuff.
Projects
None yet
2 participants