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

1862/fix setprops on cdu #2007

Merged
merged 1 commit into from Feb 6, 2019
Merged

1862/fix setprops on cdu #2007

merged 1 commit into from Feb 6, 2019

Conversation

ghost
Copy link

@ghost ghost commented Feb 6, 2019

Fixes #1862

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks, this is great!

@ghost
Copy link
Author

ghost commented Feb 6, 2019

No problem!!

@ljharb ljharb merged commit 89df015 into enzymejs:master Feb 6, 2019
@willdurand
Copy link

We noticed a regression in our test suite (between Enzyme 3.8 and 3.9), likely due to this patch. Downgrading to Enzyme 3.8 makes our test suite pass again.

We have some code that changes a component's state in componentWillReceiveProps() and, depending on this new state, a function is called in componentDidUpdate(). This is not working anymore and I believe this patch is the "culprit".

This PR shows the regression as well as the revert to Enzyme 3.8 to get a green CI again: mozilla/addons-frontend#7603.

I am not sure what you or I could do though. Moving away from CWRP is on our roadmap and might "fix" this incompatibility, but upgrading from 3.8 to 3.9 should not break our test suite. I don't know what's the most "correct" behavior between no batch updates, the <=3.8 behavior and the new one introduced by this patch 🤷‍♂️

@ljharb
Copy link
Member

ljharb commented Feb 20, 2019

@willdurand thanks for the report, could you file a new issue and fill out the template?

(Note that whatever react does is going to be "correct"; so it seems like 3.9 either fixed a bug, or introduced one)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants