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

WIP: Replace BaseInput with BaseMetaInput #2585

Draft
wants to merge 1 commit into
base: unstable
Choose a base branch
from

Conversation

fatbattk
Copy link
Contributor

@fatbattk fatbattk commented Jan 24, 2025

Background

Spawned from, https://github.com/Shopify/pos-next-react-native/pull/51443 which automatically prepends BaseInput data to every worker run call on POS.

Goal here is so there is a clear separation for,

  1. event target input types from,
  2. the "meta data" input types.

Coupling them makes it difficult to separate type concerns on POS.

Solution

  • Move BaseInput properties to BaseMetaInput.
  • Left BaseInput as placeholder for all event targets *Inputs.
  • The BaseMetaInput types are not needed as arguments for ExtensionRunner.run(), only for UnifiedExtensionWorker.run().
  • With this PR, typing on POS is simpler with ExtensionRunner.run() referencing the correct *Inputs type, and optionally UnifiedExtensionWorker.run() can reference the combined typings (currently typed as any).

🎩

  1. Check out this branch locally.
  2. On POS RN, run ./scripts/ui-extensions/yalc-local-extensions add.
  3. Validate on IDE lint. This should not have type errors.
    // example:
    ExtensionRunner.run('pos.cash-tracking-session-start.event.observe', {
      cashTrackingSessionStart: {
        id: getId(cashTrackingSession.id),
        openingTime: cashTrackingSession.openingTime,
      },
    });
Before After
image image

Checklist

  • I have 🎩'd these changes
  • I have updated relevant documentation

@fatbattk fatbattk requested review from ProdigyXable, js-goupil, Alex-Palad and a team January 24, 2025 18:29
@fatbattk fatbattk self-assigned this Jan 24, 2025
@fatbattk fatbattk requested review from friyiajr and removed request for a team January 24, 2025 18:29
Copy link
Contributor

We detected some changes in packages/*/package.json or packages/*/src, and there are no updates in the .changeset directory. If the changes are user-facing and should cause a version bump, run yarn changeset to track your changes and include them in the next release CHANGELOG. If you are making simple updates to repo configuration, examples, or documentation, you do not need to add a changeset.

@js-goupil
Copy link
Contributor

I think there's some confusion. The metadata in BaseInput that is being moved over is and should be passed in for all targets. It provides important information on the shop, user, and overall session. In what context would we not need it ?

@fatbattk
Copy link
Contributor Author

fatbattk commented Jan 24, 2025

I think there's some confusion. The metadata in BaseInput that is being moved over is and should be passed in for all targets. It provides important information on the shop, user, and overall session. In what context would we not need it ?

so this is to separate the typing. eventually all data is passed to the webworker.

Types
*Input do not need the type/info when using event targets on POS -> ExtensionRunner.run
*Input + BaseMetaInput ExtensionRunner.run -> UnifiedExtensionWorker.run

@fatbattk fatbattk marked this pull request as draft January 24, 2025 19:26
@fatbattk
Copy link
Contributor Author

Pausing since input not only represents the type for .run but also the input returned to partners via extensions(). BaseInput data must be returned to partners too.
Will revisit to split typing after OOO.

@fatbattk fatbattk changed the title Replace BaseInput with BaseMetaInput WIP: Replace BaseInput with BaseMetaInput Jan 24, 2025
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.

2 participants