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

Use googlesitekit-data imports directly instead of object destructing #8769

Closed
3 of 4 tasks
tofumatt opened this issue May 29, 2024 · 3 comments
Closed
3 of 4 tasks
Labels
javascript Pull requests that update Javascript code P1 Medium priority Team S Issues for Squad 1 Type: Infrastructure Engineering infrastructure & tooling

Comments

@tofumatt
Copy link
Collaborator

tofumatt commented May 29, 2024

Feature Description

Our googlesitekit-* modules (eg. googlesitekit-data, googlesitekit-api, etc.) are exported as objects with only default entrypoints, meaning to use things like useSelect or withSelect, we need to import the whole module, then do object destructuring to get the actual function we want, eg:

import Data from 'googlesitekit-data';

const { createRegistrySelector, useSelect } = Data;

These webpack aliases are unlike any other code/imports in our codebase—they're sort of awkward, don't allow for auto-imports, and make following the path to 'googlesitekit-*' modules in VSCode/other TypeScript-aware editors impossible, because they aren’t aliased.

This would be a nicer interface:

import { createRegistrySelector, useSelect } from 'googlesitekit-data';

I forget why we didn't do this at the time, but it might've just been an error of omission that's ballooned to quite a big surface area of a small pain point 😆

We should support importing directly from the module. My proof-of-concept PR removes about 300 lines of code and improves the developer ergonomics/experience a fair bit in doing so: #8753.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • All properties of the Data module that is available as googlesitekit.data should also be exported as named exports from googlesitekit-data, so our internal code can access these functions/constants directly as named imports instead of properties from the Data object.
  • It should be possible to follow the import 'googlesitekit-data' in VSCode/other TypeScript-aware tooling to the file that exports these functions, eg. assets/js/googlesitekit/data/index.js.

Implementation Brief

Note: Leave this issue for @tofumatt to work on, as he's already started the proof-of-concept PR for this issue and has a lot of context, as he worked on it during the Hackathon 🙂

See: #8753 for the proof-of-concept for this PR.

Test Coverage

  • No new tests are needed, but all existing tests should pass without modification.

QA Brief

No explicit QA instructions; this modifies all of our datastore imports, but should not actually change any behaviour.

It would be best to set up a site with all modules and do basic smoke testing after this is merged. If there are no issues, everything is good 😄

Changelog entry

  • N/A
@tofumatt tofumatt added P1 Medium priority Type: Infrastructure Engineering infrastructure & tooling javascript Pull requests that update Javascript code labels May 29, 2024
@tofumatt tofumatt self-assigned this May 29, 2024
@tofumatt tofumatt removed their assignment Jun 11, 2024
@binnieshah binnieshah added Team S Issues for Squad 1 Next Up Issues to prioritize for definition labels Jun 12, 2024
@eugene-manuilov eugene-manuilov self-assigned this Jun 19, 2024
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

I was able to set up a site with all modules and completed basic smoke testing. I could not find any obvious issues. No console errors, or on screen error messages. We will ensure that more through testing is completed during release testing.

@wpdarren wpdarren removed their assignment Jun 25, 2024
@aaemnnosttv
Copy link
Collaborator

We have one instance of this left that looks like it was added while this one was in flight in

We can clean it up later though 👍

@tofumatt can we perhaps include this in one of the follow-up issues to do the other packages?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code P1 Medium priority Team S Issues for Squad 1 Type: Infrastructure Engineering infrastructure & tooling
Projects
None yet
Development

No branches or pull requests

6 participants