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

Start refactor to provider check for emit instead of SCU Consum… #1931

Merged
merged 8 commits into from
Sep 22, 2019

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Sep 14, 2019

I've started this but will probably be getting back to it next week, Bad news I've found a potential stale dom issue with the SCU I've added in the tests

Fixes #1933

EDIT: Add list of resolved issues - @marvinhagemeister

@coveralls
Copy link

coveralls commented Sep 16, 2019

Coverage Status

Coverage increased (+0.1%) to 99.882% when pulling 6d88906 on feat/checkForNeedToEmit into 8c6ac43 on master.

@mxstbr
Copy link
Contributor

mxstbr commented Sep 16, 2019

Note to readers, here is the most minimal reproduction of the issue: https://codesandbox.io/s/double-context-preventing-value-propagation-rj00e

@cristianbote
Copy link
Member

Hey @mxstbr thanks for the sandbox. The thing is, that with master that works. Got merged in here https://github.com/preactjs/preact/pull/1925/files#diff-e959fc84d230f763daeb8f8fa0a571e7R18. Is there another case where this breaks?

@mxstbr
Copy link
Contributor

mxstbr commented Sep 16, 2019

I am running the latest master in our project locally and it is definitely still broken. Here is the slightly-less-minimal reproduction that I sent @JoviDeCroock privately: https://codesandbox.io/s/issue-1864-urv9j

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

Looking good here👍

test/browser/createContext.test.js Outdated Show resolved Hide resolved
@atcq-me
Copy link

atcq-me commented Sep 19, 2019

Hi, should this pull request fix issue #1933?

@marvinhagemeister
Copy link
Member

@Nuzsk Just checked the repro in the linked issue with the changes in this PR and it fixes the issue for me. The popup appears as expected with this PR 👍

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

Just checked all issues that relate to this that I know of and the changes here resolve them. In my opinion this PR is ready to be merged 💯

@JoviDeCroock JoviDeCroock marked this pull request as ready for review September 22, 2019 06:43
Copy link
Member Author

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

Thank you so much for taking this up @cristianbote! Hero

@marvinhagemeister
Copy link
Member

Let-a-go 🍄 👍

@marvinhagemeister marvinhagemeister changed the title Start refactor to provider check for emit instead of SCU Consumer Start refactor to provider check for emit instead of SCU Consum… Sep 22, 2019
@marvinhagemeister marvinhagemeister merged commit 9eba4e0 into master Sep 22, 2019
@marvinhagemeister marvinhagemeister deleted the feat/checkForNeedToEmit branch September 22, 2019 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10.0.0-rc.2 break antd/popover
6 participants