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

Speed up patching 5-10x #2845

Merged
merged 20 commits into from
Oct 13, 2023
Merged

Speed up patching 5-10x #2845

merged 20 commits into from
Oct 13, 2023

Conversation

chrismccord
Copy link
Member

No description provided.

@chrismccord chrismccord requested a review from josevalim October 4, 2023 20:46
@jswanner
Copy link
Contributor

jswanner commented Oct 5, 2023

@chrismccord, tags with phx-remove are being reported in the console as duplicating IDs when using this branch:

utils.js:19 Multiple IDs detected: filter-modal. Ensure unique element ids.

@chrismccord chrismccord requested a review from josevalim October 11, 2023 17:39
josevalim
josevalim reviewed Oct 11, 2023
View reviewed changes
assets/js/phoenix_live_view/rendered.js
// experienced a change which must be a newRender, and thus we can't skip the render.
//
// Both optimization flows apply here. newRender is set based on the onlyCids optimization, and
// we track a deterministic magicId based on the cid.
Copy link
Member

Choose a reason for hiding this comment

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

👏 👏 👏

josevalim
josevalim approved these changes Oct 11, 2023
View reviewed changes
Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

Glorious, I have added some comments. Feel free to incorporate or discard them.

@josevalim josevalim changed the title Speed up patching 5x Speed up patching 5-10x Oct 12, 2023
@bcardarella
Copy link
Contributor

Is this considered a backwards incompatible change? We're assessing it for LVN. I suspect if a LV server updates they'll need a new version of the LVN client

@josevalim
Copy link
Member

No, the server changes are minimum (1 LOC) and you can safely ignore the additional parameter.

@chrismccord chrismccord merged commit 3d28c02 into main Oct 13, 2023
8 checks passed
@chrismccord
Copy link
Member Author

❤️❤️❤️🐥🔥

@chrismccord chrismccord deleted the cm-optimize-patch branch October 13, 2023 12:48
chrismccord added a commit that referenced this pull request Oct 13, 2023
@chrismccord @josevalim
Speed up patching 5-10x (#2845)
bed2ff0 
Co-authored-by: José Valim <[email protected]>
@DaTrader DaTrader mentioned this pull request Feb 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vanderhoop vanderhoop vanderhoop left review comments

@josevalim josevalim josevalim 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.
5 participants
@chrismccord @jswanner @bcardarella @josevalim @vanderhoop