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

Hydration #38

Open
wants to merge 50 commits into
base: master
Choose a base branch
from
Open

Hydration #38

wants to merge 50 commits into from

Conversation

srghma
Copy link

@srghma srghma commented May 13, 2020

fixes #37

WIP

@natefaubion
Copy link
Collaborator

natefaubion commented May 13, 2020

I would appreciate it if we could avoid major refactors. It makes it difficult to audit what has actually changed.

@srghma
Copy link
Author

srghma commented May 13, 2020

I would appreciate it if we could avoid major refactors. It makes it difficult to audit what has actually changed.

ok, moved only refactoring changes to #39

@natefaubion
Copy link
Collaborator

My suggestion is to work on adding hydration with the minimal number of changes necessary to the existing code, written in the style of the existing code. A useful new feature should not be predicated on a largely unnecessary and opinionated refactoring. I understand these opinions, and I appreciate the reasoning behind them, but it is very unlikely that I'm going to merge that PR. I do not want to reformat, rewrite, and refactor for the sake of it.

@srghma
Copy link
Author

srghma commented Jun 10, 2020

I have reverted #b385b50 in commit c17301b

@srghma
Copy link
Author

srghma commented Jun 24, 2020

@natefaubion @thomashoneyman @garyb The pr is ready

it is in sync with purescript-halogen/purescript-halogen#671

I have tested it using this project https://github.com/srghma/purescript-halogen-nextjs/ that is using a copy of examples from halogen, plus additional ones (DeepNesting and TextNodes)

The pr in halogen is using next-6 branch, but all latest changes from master are merged in there

@fauna-brecht
Copy link

@srghma
I know you are still waiting for the merge etc, but I was looking into Halogen out of interest and wondered whether it supported hydration. Therefore, I stumbled upon your PR and wondered what it supported if you don't mind.
Will it be able to start from a partial static DOM and add dynamic items to it and modify existing items that have changed slightly or is it only meant to add event listeners? I'm hoping to use it in a hobby project when it eventually becomes available, thanks for adding this 👍

@srghma
Copy link
Author

srghma commented Jan 10, 2021

modify existing items that have changed slightly

no, it will throw error IF dom doesnt match vdom

in future would be good to do this check in development, but omit in production

or is it only meant to add event listeners?

yes, and also split text nodes ("text1text2" -> "text1""text2" / add empty one "")

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.

Hydration
3 participants