Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

Add priorities to directives #235

Merged
merged 11 commits into from
May 23, 2023

Conversation

DAreRodz
Copy link
Collaborator

What

This PR extends the directive() function API to add a new option called priority, making directives on the same element to be evaluated in priority order. In such an element, directives with the same priority are evaluated together, wrapped in the same component, and those with less priority are wrapped in a nested component.

Priorities follow the same schema as in WordPress, i.e., priority is a numeric value, lower numbers correspond with earlier execution ("more" priority), and the default value is 10.

Note that this value is per directive type, as it is set during directive definition. It doesn't allow changing the order in which directives are evaluated on different parts of the HTML.

A trivial example:

directive(
  'customdirective',
  ({ directives: { customdirective }) => { /* ... */ },
  { priority: 12 } // arbitrary value, just for display purposes
);

In addition, the priority for the data-wp-context directive has been set to 5.

Why

Priorities allow directives to manipulate the element in which they appear, in the correct order. It is especially convenient for directives that add wrappers, like a context Provider (e.g. data-wp-context), allowing other directives with less priority in the same element to consume that context.

How

The main changes are in /src/runtime/hooks.js:

  • I've implemented a hook called usePriorityLevels, that receives the object with all directives defined in an element and returns them sorted and grouped by priority.
  • I'm using a recursive component called RecursivePriorityLevel inside the Directive component. It renders a component instance for each priority level that usePriorityLevels yields.

Copy link
Contributor

@SantosGuillamot SantosGuillamot left a comment

Choose a reason for hiding this comment

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

Approving from my side because I tested and it seems to work great 🙂 (actually, it can help fixing this issue).

I just tested that it works. Please wait for a proper code review before merging it.

Copy link
Member

@luisherranz luisherranz left a comment

Choose a reason for hiding this comment

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

Looks good to me as well. Great work, David 🙂

src/runtime/hooks.js Outdated Show resolved Hide resolved
Comment on lines 87 to 90
// This element needs to be a fresh copy so we are not modifying an already
// rendered element. This prevents an error with changes in
// `element.props.children` not being reflected in `element.__k`.
element = cloneElement(element);
Copy link
Member

Choose a reason for hiding this comment

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

@DAreRodz, can you explain this in more detail? Why is element already rendered?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The thing is that element comes from the Directive component, which never re-renders, so element is always the same instance. If we don't clone it, it's unsafe to modify its properties the way we do inside directives because some element's internals wouldn't be updated (like the __k property).

src/runtime/hooks.js Outdated Show resolved Hide resolved
e2e/js/directive-priorities.js Outdated Show resolved Hide resolved
e2e/specs/directive-priorities.spec.ts Show resolved Hide resolved
DAreRodz and others added 3 commits May 23, 2023 10:50
Co-authored-by: Luis Herranz <luisherranz@gmail.com>
Co-authored-by: Luis Herranz <luisherranz@gmail.com>
@DAreRodz DAreRodz merged commit b0ad75f into main-wp-directives-plugin May 23, 2023
@DAreRodz DAreRodz deleted the directive-priorities-recursive branch May 23, 2023 09:14
@DAreRodz
Copy link
Collaborator Author

Thanks for the reviews. PR is merged now. 🙌

As a follow-up, we can try improving the code with these changes:

  • Move the logic inside the usePriorityLevels hook to the vnode option hook.
  • Refactor components so we don't need an extra component (Directive)
  • Show directive names for each RecursivePriorityLevel in the preact dev tools

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants