Skip to content

Conversation

@buntec
Copy link
Owner

@buntec buntec commented May 27, 2022

Experiments in making vnodes immutable.

@buntec
Copy link
Owner Author

buntec commented May 27, 2022

This is currently broken despite all tests passing. Yikes!

@buntec
Copy link
Owner Author

buntec commented May 27, 2022

Looks better now, though the fact that the tests didn't catch these issues is disconcerting!

@buntec
Copy link
Owner Author

buntec commented May 27, 2022

Tests are passing and some ff4s examples look ok, too. The biggest concern is probably performance, although the todo-mvc benchmark using ff4s barely budged after the change. Not sure what to make of that.

@buntec buntec marked this pull request as ready for review May 27, 2022 16:01
@buntec buntec requested a review from armanbilge May 27, 2022 16:01
@armanbilge
Copy link
Collaborator

though the fact that the tests didn't catch these issues is disconcerting!

This would be my concern, would be nice to add some tests for these.

Unfortunately I don't think I can do an in-depth review of this one, but if it works it works :) 👍

@buntec
Copy link
Owner Author

buntec commented May 27, 2022

though the fact that the tests didn't catch these issues is disconcerting!

This would be my concern, would be nice to add some tests for these.

Unfortunately I don't think I can do an in-depth review of this one, but if it works it works :) 👍

No worries! I'm aware that this is a big change, and one that marks a clear departure from the original snabbdom. Any high level feedback would still be appreciated ;). Having immutable vnodes is one of the things mentioned by @cornerman in this issue.

I'll try to increase test coverage to include the cases I came across when making these changes.

@buntec buntec marked this pull request as draft May 28, 2022 07:48
@buntec buntec mentioned this pull request Jun 25, 2022
@buntec buntec merged commit 72574ae into main Jul 4, 2022
@buntec buntec deleted the immutable-vnodes branch July 4, 2022 18:40
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.

3 participants