⚛️ Refactor directives and make them work inside client components #74

luisherranz · 2022-09-22T09:50:17Z

Fixes #73.

For now, I've just added a simple directive to reproduce the problem. I'll continue working on the solution.

luisherranz · 2022-09-22T11:12:16Z

I don't like having to traverse all the props again in options.vnode to search for the ones that start with wp-, but I don't see another way around it. Other ideas are welcomed 🙂

luisherranz · 2022-09-22T12:27:55Z

Well, there are actually options to avoid the double traverse:

Marking the static vnodes to bypass the traverse in the options hooks.
Attach the attributes to the vnode and move all the traverse to the options hooks.

I think I like the first option better because with the second the static vnodes won't have props until our options hook runs.

luisherranz · 2022-09-22T13:15:44Z

For future reference, a couple of perf tests:

I tested it in Chrome desktop and mobile, Firefox desktop and mobile and Safari desktop.

In Chrome, forEach is slightly faster but not for much.
In Safari, forEach is quite slow. for and for of are similar.
In Firefox, for is faster.

for seems to be the best option in both cases.

luisherranz · 2022-09-26T16:08:44Z

I think everything is finished here. Ready for review.

https://www.loom.com/share/4f9809e14edc45b49024014a92086862

luisherranz · 2022-09-26T16:17:46Z

Oh, something is going on with the block context. It doesn't work after hiding+showing. I'll take a look.

Screen.Capture.on.2022-09-26.at.18-15-39.mp4

DAreRodz · 2022-09-26T17:31:14Z

Oh, something is going on with the block context. It doesn't work after hiding+showing. I'll take a look.

~~Hey @luisherranz, I tried to reproduce the issue you mentioned, but after hiding and showing the blocks, the context still works. The blocks were arranged similarly. 😕~~

I also tested it in main-directives-hydration, just in case, and it also worked.

EDIT: my bad, I was in the wrong commit (9057066). 🤦 In the latest commit, the issue is reproducible.

DAreRodz

@luisherranz, the code seems great. 👍

Regarding the issue, I don't know where it could be TBH. 😕 Maybe it is related to the attributes being processed now inside the wpType directive, or re-processing the wp directives on non-static nodes...

No idea. 🙃

DAreRodz · 2022-09-27T07:25:08Z

I've taken another quick look at the block context issue, and I think it should be related to the way sourced attributes are computed after the refactoring (inside the blockType directive).

It is like the title attribute disappears when the node is mounted again.

luisherranz · 2022-09-27T07:25:45Z

Thanks, David. I'll take a look later today 🙂

luisherranz · 2022-09-28T10:20:57Z

It is like the title attribute disappears when the node is mounted again.

It disappears because it's a sourced attribute and needs to look at the DOM to get the value. At the beginning of the application, we have a DOM and create a vDOM. So we go DOM-> vDOM and the sourced attribute values can be found. But when Preact components add nodes, we go in the other direction, vDOM-> DOM. So there's no DOM to search for the sourced attribute values.

I'm not sure why or how this was working previously. I'll take a look.

luisherranz · 2022-09-28T10:29:13Z

I'm not sure why or how this was working previously. I'll take a look.

Ok, it makes sense now. You were processing the sourced attributes in the toVdom function, which means they will exist (and persist) in the vnode children (vnode.props.children) that then the component hides/shows using {show && children}. I'm processing them in the options.vnode function, which means they are processed on the fly each time a vnode is created, and therefore it needs to process them again when there's no DOM yet.

I'll think about how we can do this without hardcoding that behavior into the toVdom function.

To see them in the Preact devtools.

…ient-components

luisherranz · 2022-09-28T16:22:10Z

I've finally exposed the vnode and I'm using it a storage medium for the source attributes. Of course I don't love it, but it's the only thing that is stable and at least it's not hardcoded. We can think about better alternatives in the future.

I've also added support for returning components.

This should be ready for review!

https://www.loom.com/share/e71a9867720542eaa5812d70741ae4fd

cc: @DAreRodz

luisherranz · 2022-09-29T07:18:20Z

EDIT: The video is finally loading fine.

DAreRodz

Great refactoring, thanks @luisherranz!! 👏

PS: I left a comment below.

DAreRodz · 2022-09-29T09:00:40Z

src/directives/wp-block.js

+	if (!vnode._blockAttributes) {
+		vnode._blockAttributes = blockAttributes || {};
+		useMemo(() => {
+			for (const attr in blockSourcedAttributes) {
+				vnode._blockAttributes[attr] = matcherFromSource(
+					blockSourcedAttributes[attr]
+				)(initialRef);
+			}
+		}, [blockAttributes, blockSourcedAttributes]);
+	}
+	props.attributes = vnode._blockAttributes;


I was intrigued by that useMemo, and I think we may be able to get rid of it.

We are using vnode as a container to store_blockAttributes. That vnode —the original one returned by toVdom()— is not recreated and always exists. So, once you set the _blockAttributes prop, I guess that code won't run again, am I right? 🤔

Apart from that, useMemo is used conditionally, and that's not supposed to be the way. 😅

You're totally right. Fixed.

I've also added a comment to explain why we're doing such a weird thing.

I'm also curious how sourced attributes will play out with the initially hidden blocks (the ones that will use <template> in the future).

luisherranz · 2022-09-29T09:45:45Z

Thanks, David!! 🎉

Add wp-log directive to test this

9057066

luisherranz marked this pull request as draft September 22, 2022 09:50

luisherranz linked an issue Sep 22, 2022 that may be closed by this pull request

⚛️ Directives don't work if they are used inside of a component #73

Closed

This was referenced Sep 22, 2022

⚛️ Directives don't work if they are used inside of a component #73

Closed

Tracking issue: Directives Hydration ⚛️ #64

Closed

Refactor to use wp-

5104a18

luisherranz marked this pull request as ready for review September 26, 2022 16:08

luisherranz requested a review from DAreRodz September 26, 2022 16:08

luisherranz changed the title ~~Refactor directives and make them work inside client components~~ ⚛️ Refactor directives and make them work inside client components Sep 26, 2022

luisherranz self-assigned this Sep 26, 2022

DAreRodz reviewed Sep 26, 2022

View reviewed changes

luisherranz added 3 commits September 28, 2022 17:31

Rename View components

9ce147c

To see them in the Preact devtools.

Support returning components and expose vnode

e28962e

Merge branch 'main-full-vdom-hydration' into fix-directives-inside-cl…

37825f9

…ient-components

luisherranz requested a review from DAreRodz September 28, 2022 16:22

luisherranz added 3 commits September 29, 2022 10:44

Rename blockType directive file

c11e8ce

Get context values from props.attributes

4604bca

Rename blockProvidesBlockContext directive file

23c1482

DAreRodz approved these changes Sep 29, 2022

View reviewed changes

Remove useMemo and add a comment to explain vnode storage

ff42a65

luisherranz merged commit cc0982e into main-full-vdom-hydration Sep 29, 2022

luisherranz deleted the fix-directives-inside-client-components branch September 29, 2022 09:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚛️ Refactor directives and make them work inside client components #74

⚛️ Refactor directives and make them work inside client components #74

luisherranz commented Sep 22, 2022

luisherranz commented Sep 22, 2022

luisherranz commented Sep 22, 2022

luisherranz commented Sep 22, 2022 •

edited

luisherranz commented Sep 26, 2022

luisherranz commented Sep 26, 2022

DAreRodz commented Sep 26, 2022 •

edited by luisherranz

DAreRodz left a comment

DAreRodz commented Sep 27, 2022

luisherranz commented Sep 27, 2022

luisherranz commented Sep 28, 2022 •

edited

luisherranz commented Sep 28, 2022

luisherranz commented Sep 28, 2022

luisherranz commented Sep 29, 2022 •

edited

DAreRodz left a comment

DAreRodz Sep 29, 2022

luisherranz Sep 29, 2022

luisherranz commented Sep 29, 2022

⚛️ Refactor directives and make them work inside client components #74

⚛️ Refactor directives and make them work inside client components #74

Conversation

luisherranz commented Sep 22, 2022

luisherranz commented Sep 22, 2022

luisherranz commented Sep 22, 2022

luisherranz commented Sep 22, 2022 • edited

luisherranz commented Sep 26, 2022

luisherranz commented Sep 26, 2022

DAreRodz commented Sep 26, 2022 • edited by luisherranz

DAreRodz left a comment

Choose a reason for hiding this comment

DAreRodz commented Sep 27, 2022

luisherranz commented Sep 27, 2022

luisherranz commented Sep 28, 2022 • edited

luisherranz commented Sep 28, 2022

luisherranz commented Sep 28, 2022

luisherranz commented Sep 29, 2022 • edited

DAreRodz left a comment

Choose a reason for hiding this comment

DAreRodz Sep 29, 2022

Choose a reason for hiding this comment

luisherranz Sep 29, 2022

Choose a reason for hiding this comment

luisherranz commented Sep 29, 2022

luisherranz commented Sep 22, 2022 •

edited

DAreRodz commented Sep 26, 2022 •

edited by luisherranz

luisherranz commented Sep 28, 2022 •

edited

luisherranz commented Sep 29, 2022 •

edited