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

fix(history-queries): ensuring history-queries x-module registration before using components #1512

Conversation

joseacabaneros
Copy link
Member

@joseacabaneros joseacabaneros commented Jun 10, 2024

Precedents

  • xComponentMixin(historyQueriesXModule) mixin is executed as soon as the component is imported, before mounting it.
  • useRegisterXModule(historyQueriesXModule) composable is executed once the component is mounted.
  • Sometimes in setups, we need to check some state values before mounting the component (v-if). So using the composable, the XModule has not been registered when the check is executed because the component has not been mounted yet.

How can we ensure the XModule registration?

Registering it as soon as we can, in the XModule creation itself.

Key concepts to understand why it works

  • Every component must have an import of its XModule to assign the xModule name to itself.

A module code is evaluated only the first time when imported.
If the same module is imported into multiple other modules, its code is executed only once, upon the first import. Then its exports are given to all further importers
https://javascript.info/modules-intro#a-module-code-is-evaluated-only-the-first-time-when-imported

Workflow example

  1. In a setup, we import a xModule component: import { HistoryQueries } from '@empathyco/x-components/history-queries';
  2. HistoryQueries component imports the module import { historyQueriesXModule } from '../x-module' to assign the xModule name to the component xModule: historyQueriesXModule.name
  3. In the xModule JS file, we register the component itself after creating it XPlugin.registerXModule(historyQueriesXModule);
  4. The registration is done as soon as the ES module is imported, and it is only executed and registered the first time the xModule is imported (with the mixin or composable, the registration was tried once per component)

tl;dr

xModule registration now is done at the same time an X component is imported, and we can guarantee the store module is registered before mounting the component.
We just need to remember to assign the xModule name to each x-component xModule: historyQueriesXModule.name

@@ -23,3 +24,5 @@ export const historyQueriesXModule: HistoryQueriesXModule = {
storeEmitters: historyQueriesEmitters,
wiring: historyQueriesWiring
};

XPlugin.registerXModule(historyQueriesXModule);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we document how we should use a module?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, but probably not on this PR IMO

@joseacabaneros joseacabaneros marked this pull request as ready for review June 11, 2024 08:10
@joseacabaneros joseacabaneros requested a review from a team as a code owner June 11, 2024 08:10
@@ -23,3 +24,5 @@ export const historyQueriesXModule: HistoryQueriesXModule = {
storeEmitters: historyQueriesEmitters,
wiring: historyQueriesWiring
};

XPlugin.registerXModule(historyQueriesXModule);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, but probably not on this PR IMO

@diegopf diegopf merged commit e3bb670 into feature/EMP-4164-migrate-history-queries-x-module-to-composition-api Jun 11, 2024
2 checks passed
@diegopf diegopf deleted the register-history-queries-x-module-when-import branch June 11, 2024 09:11
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