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.

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

Jump to bottom

(refactor) move context update logic to diffing #1468

Merged
merged 15 commits into from Mar 30, 2019
Merged

(refactor) move context update logic to diffing #1468

merged 15 commits into from Mar 30, 2019

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Mar 27, 2019 •
edited

I've made a first draft since it seems to solve the issue adressed by @marvinhagemeister with useContext.
Added the test in his branch here, and it works.

Will now look at how we can implement the others.

Size change: +26B +17B -3B -10B
Fixes #1363

andrewiggins
andrewiggins reviewed Mar 27, 2019
View reviewed changes
src/diff/index.js Outdated Show resolved Hide resolved
@JoviDeCroock JoviDeCroock changed the title (refactor) move context logic to diffing (refactor) move context update logic to diffing Mar 28, 2019
@JoviDeCroock
Copy link
Member Author

Added some tests to see if this would introduce bugs in Context.Consumer but those come out positive aswell.

@JoviDeCroock JoviDeCroock marked this pull request as ready for review March 28, 2019 16:19
JoviDeCroock
JoviDeCroock commented Mar 28, 2019
View reviewed changes
src/diff/index.js Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Mar 28, 2019 •
edited

Coverage Status

Coverage remained the same at 100.0% when pulling 6597287 on JoviDeCroock:refactor/contextLogicDiffing into 76c4918 on developit:master.

@JoviDeCroock
Copy link
Member Author

JoviDeCroock commented Mar 30, 2019 •
edited

Another approach could be to instead of use componentDidUpdate to use shouldComponentUpdate to notify subs, how are you standing on this?

In comparison to doing a willUpdate during diffing this instead of adds bytes saves them + solves our issue.

Approach in diffing: f77f265

@marvinhagemeister
Copy link
Member

@JoviDeCroock can you check your IDE settings? There are lots of mixed spaces and tabs as indentation in the same file 🙂

marvinhagemeister
marvinhagemeister approved these changes Mar 30, 2019
View reviewed changes
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.

Love it, the PR just got better and better! On top of that who can resist the byte savings 🎉 💯

@marvinhagemeister marvinhagemeister merged commit b84b873 into preactjs:master Mar 30, 2019
@JoviDeCroock JoviDeCroock deleted the refactor/contextLogicDiffing branch March 30, 2019 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewiggins andrewiggins andrewiggins left review comments

@marvinhagemeister marvinhagemeister marvinhagemeister approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JoviDeCroock @coveralls @marvinhagemeister @andrewiggins