Interactivity API: refactor directives into self-registering modules - #79975
Conversation
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @nickchomey! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
77c3163 to
746faa0
Compare
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
1 similar comment
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
DAreRodz
left a comment
There was a problem hiding this comment.
Hi, @nickchomey! 👋 I'm not opposed to moving each directive to its own file; that would help organizing the repo. However, I'd prefer if you modify less files.
You could create an index.ts file inside /directives, with it being in charge of importing all directives and exporting a function that does the same as the ' directives. tsx ' file currently does. That way we don't need to introduce changes in the runtime initialization.
Thanks! 🙂
d19ed81 to
09f6874
Compare
|
Thanks for the review and sorry for the overreach! I knew that would be a problem, so should have just put those changes in a later commit to make it easier to revert. Anyway, I believe it now looks how you requested. Please let me know if you'd like to see anything else change. |
DAreRodz
left a comment
There was a problem hiding this comment.
Thanks, @nickchomey! Now that the scope of the changes is narrow, I'm paying more attention to them. 🙂
I left some comments below. Just address them and the PR is ready to go.
| } | ||
|
|
||
| // Preserve the initial inner HTML | ||
| const cached = useMemo( () => innerHTML, [ innerHTML ] ); |
There was a problem hiding this comment.
Oops, it seems like this directive's behavior has changed. 👀
What this line should do is to preserve the initial value. If we introduce innerHTML as a useMemo's dependency, the cached value would be mistakenly updated.
It should be this instead. It could require adding // eslint-disable-next-line react-hooks/exhaustive-deps to prevent a linting error.
| const cached = useMemo( () => innerHTML, [ innerHTML ] ); | |
| const cached = useMemo( () => innerHTML, [] ); |
There was a problem hiding this comment.
Yes, i added that because of the linting error. Was silly to incorporate it as part of this PR, and especially without its own commit...
Should there be a test somewhere that would have caught any behavioural changes from that addition? Or perhaps now that it has the lint comment it isnt necessary. Plus it is already a deprecated directive...
e09e05d to
7f7eee2
Compare
|
I think I've addressed it all now |
|
@nickchomey, I wanted to make a small change before merging the PR, but I don't have permission to do so in your fork. Basically, the change removes the Could you please apply this patch? Ping me, and I can merge the PR afterward. 🙂 diff --git a/packages/interactivity/src/directives/index.ts b/packages/interactivity/src/directives/index.ts
index dd8132eda00..b116dd580aa 100644
--- a/packages/interactivity/src/directives/index.ts
+++ b/packages/interactivity/src/directives/index.ts
@@ -2,11 +2,9 @@
* Registers all core Interactivity API directives.
*
* Each directive file registers itself via `directive()` at module scope when
- * imported. This module aggregates all of them and exports a single
- * initialization function so that `index.ts` can call it uniformly.
+ * imported. This module aggregates all of them.
*/
-// Import each directive module so it self-registers.
import './bind';
import './class';
import './context';
@@ -19,14 +17,3 @@ import './run';
import './style';
import './text';
import './watch';
-
-// Re-export so the caller can reference the same singleton.
-export { routerRegions } from './router-region';
-
-/**
- * Initializes all core directives.
- *
- * Directives register themselves at import time, so this function is a no-op
- * that exists only to satisfy the calling convention in `index.ts`.
- */
-export default function registerDirectives(): void {}
diff --git a/packages/interactivity/src/index.ts b/packages/interactivity/src/index.ts
index b12cefefa20..24b63d9fa3d 100644
--- a/packages/interactivity/src/index.ts
+++ b/packages/interactivity/src/index.ts
@@ -11,7 +11,8 @@ import { batch, effect } from '@preact/signals';
/**
* Internal dependencies
*/
-import registerDirectives, { routerRegions } from './directives';
+import './directives'; // Registers all the core directives.
+import { routerRegions } from './directives/router-region';
import {
initialVdomPromise,
hydrateRegions,
@@ -100,10 +101,8 @@ export const privateApis = (
throw new Error( 'Forbidden access.' );
};
-// Parses and populates the initial state and config. All the core directives
-// are registered at this point as well.
+// Parses and populates the initial state and config.
populateServerData( parseServerData() );
-registerDirectives();
// Hydrates all interactive regions when `DOMContentLoaded` is dispatched, or as
// soon as the `@wordpress/interactivity` module is evaluated in the case that
|
…strap.ts and index.ts becomes a clean entry point where you can specify which directives you want to use
7f7eee2 to
99e7a39
Compare
There was a problem hiding this comment.
Pull request overview
Refactors the @wordpress/interactivity directive registration so each directive lives in its own module under src/directives/ and registers itself via a module-scoped directive() call, enabling a “directives entrypoint” that can be customized for bespoke builds.
Changes:
- Replace the old
registerDirectives()flow with a side-effect import of./directivesfromsrc/index.ts. - Split the monolithic
src/directives.tsxinto per-directive modules undersrc/directives/. - Extract shared warning helpers into
src/directives/utils/warnings.ts.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/interactivity/src/index.ts | Switches directive registration to a side-effect import and removes the explicit registerDirectives() call; continues exposing routerRegions via privateApis. |
| packages/interactivity/src/directives/index.ts | New directives “entrypoint” that imports (and thus registers) all core directives. |
| packages/interactivity/src/directives/utils/warnings.ts | Centralizes shared warning helpers used by multiple directives. |
| packages/interactivity/src/directives/context.ts | data-wp-context directive moved to a dedicated self-registering module. |
| packages/interactivity/src/directives/watch.ts | data-wp-watch directive moved to a dedicated self-registering module. |
| packages/interactivity/src/directives/init.ts | data-wp-init directive moved to a dedicated self-registering module. |
| packages/interactivity/src/directives/on.ts | data-wp-on family of directives moved to a dedicated self-registering module. |
| packages/interactivity/src/directives/class.ts | data-wp-class directive moved to a dedicated self-registering module. |
| packages/interactivity/src/directives/style.ts | data-wp-style directive moved to a dedicated self-registering module. |
| packages/interactivity/src/directives/bind.ts | data-wp-bind directive moved to a dedicated self-registering module. |
| packages/interactivity/src/directives/text.ts | data-wp-text directive moved to a dedicated self-registering module. |
| packages/interactivity/src/directives/run.ts | data-wp-run directive moved to a dedicated self-registering module. |
| packages/interactivity/src/directives/each.ts | data-wp-each / data-wp-each-child directives moved to a dedicated self-registering module. |
| packages/interactivity/src/directives/ignore.ts | data-wp-ignore (deprecated) directive moved to a dedicated self-registering module. |
| packages/interactivity/src/directives/router-region.ts | data-wp-router-region directive + routerRegions map moved to a dedicated self-registering module. |
| packages/interactivity/src/directives.tsx | Removed monolithic directives file in favor of per-directive modules. |
What
Closes #79977
Refactors the Interactivity API's directive registration system so each directive lives in its own file under
src/directives/and registers itself via a module-scopeddirective()call.Why
directives.tsx.index.ts, you can create a minimal or custom build that only includes the directives you need.Changes
Directives split into individual files
src/directives.tsx(1060 lines) was split into:src/directives/context.ts—data-wp-context(priority 5)src/directives/watch.ts—data-wp-watchsrc/directives/init.ts—data-wp-initsrc/directives/on.ts—data-wp-on,on-async,on-window,on-document(and deprecated async variants)src/directives/class.ts—data-wp-classsrc/directives/style.ts—data-wp-stylesrc/directives/bind.ts—data-wp-bindsrc/directives/text.ts—data-wp-textsrc/directives/run.ts—data-wp-runsrc/directives/each.ts—data-wp-each(priority 20),data-wp-each-child(priority 1)src/directives/ignore.ts—data-wp-ignore(deprecated)src/directives/router-region.ts—data-wp-router-region(priority 1), also exportsrouterRegionsEach file calls
directive()at module scope, so importing the file is enough to register it.New files
| File | Purpose |
|
src/directives/utils/warnings.ts| Shared warning helpers (warnUniqueIdWithTwoHyphens,warnUniqueIdNotSupported,warnWithSyncEvent) ||
src/directives/index.ts| clean entry point for the directives that can be overridden.Comment out any directive import to exclude it from the build, replace them with local customizations, or add new ones.
Removed
src/directives.tsx— replaced by individual directive files + barrel imports inindex.tsReviewer notes
This is a pure refactor. No directive behavior or registration order was changed — the same
directive()calls happen in the same priority order as before. The only difference is they execute at module scope (during import resolution) rather than when the oldregisterDirectives()function was called. Sincedirective()only stores a callback in a map, the timing is irrelevant.Use of AI Tools
AI assistance: Yes
Tool(s): VSCode Copilot Chat
Model(s): Deepseek V4 Flash
Used for: Pair programmer. I guided and reviewed it all