-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
Note to readers, here is the most minimal reproduction of the issue: https://codesandbox.io/s/double-context-preventing-value-propagation-rj00e |
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? |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good here👍
Hi, should this pull request fix issue #1933? |
@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 👍 |
There was a problem hiding this 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 💯
There was a problem hiding this 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
Let-a-go 🍄 👍 |
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