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

Packages: Move api-fetch usage to new shared package #8388

Closed
6 tasks
spacedmonkey opened this issue Jul 20, 2021 · 4 comments
Closed
6 tasks

Packages: Move api-fetch usage to new shared package #8388

spacedmonkey opened this issue Jul 20, 2021 · 4 comments
Labels
P3 Nice to have Type: Enhancement New feature or improvement of an existing feature Type: Infrastructure Changes impacting testing infrastructure or build tooling

Comments

@spacedmonkey
Copy link
Contributor

spacedmonkey commented Jul 20, 2021

Feature Description

  • Move some of the code from /assets/src/edit-story/utils/base64Encode.js to /packages/api-fetch/src/
  • Move some of the code from /packages/design-system/src/utils/addQueryArgs.js to /packages/api/src/ + tests
  • export * from '@wordpress/api-fetch';
  • Update imports where necessary.
    Use @web-stories-wp/... imports for anything that is already a package, relative imports for everything else.
  • Create /packages/api-fetch/package.json referencing its dependencies
  • Create /packages/api-fetch/README.md with short documentation.

Alternatives Considered

Additional Context


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

Acceptance Criteria

Implementation Brief

@spacedmonkey spacedmonkey added the Type: Enhancement New feature or improvement of an existing feature label Jul 20, 2021
@spacedmonkey
Copy link
Contributor Author

Adding this new package, adds a new number of benefits.

  1. base64Encode.js will not longer be loaded from editor in dashboard.
  2. addQueryArgs is moved to a more sensible place out of the design system.
  3. Neither the editor / dashboard will have direct dependencies on a WordPress package.
  4. Updating apiFetch package is done in one place.

Thoughts @swissspidy ?

@spacedmonkey spacedmonkey added P3 Nice to have Pod: WP & Infra Type: Infrastructure Changes impacting testing infrastructure or build tooling labels Jul 20, 2021
@swissspidy
Copy link
Collaborator

With the decoupling work we'll end up with at least the following packages:

  • dashboard
  • wp-dashboard
  • story-editor (think @wordpress/block-editor in Gutenberg)
  • wp-story-editor (think @wordpress/edit-post in Gutenberg)

There will probably be a fifth one with shared utilities used by both wp- packages, e.g. wp-story-utils or similar.

addQueryArgs, @wordpress/api-fetch usage, base64Encode, getResourceFromAttachment, etc. would all live in that package.

story-editor and dashboard would never use those directly. Only wp-dashboard and wp-story-editor would.

So I'd rather name this wp-story-utils or similar, not api-fetch.

@spacedmonkey
Copy link
Contributor Author

The decoupling work would be part of the reason we could do this. I am hoping that are some point, if when we decouple this editor from WordPress, we could simple replace the api-fetch package with api-fetch-drupal or similar. As long as the functions returned as the same return and params, the internals could be very different.

wp-story-utils as a name is bad, as utils is sounds like a catch-all dumping ground name. Something with api in the name makes it much clearer it's use.

@swissspidy
Copy link
Collaborator

swissspidy commented Jul 20, 2021

The decoupling work would be part of the reason we could do this. I am hoping that are some point, if when we decouple this editor from WordPress, we could simple replace the api-fetch package with api-fetch-drupal or similar. As long as the functions returned as the same return and params, the internals could be very different.

In such a scenario we would create a dedicated drupal-story-editor package that interacts with the Drupal API and passes the necessary data to story-editor, and more.

wp-story-utils as a name is bad, as utils is sounds like a catch-all dumping ground name. Something with api in the name makes it much clearer it's use.

Ignore the name, I just made this up on the spot to try to explain the goal. The point is that there would be a shared package consumed by the two wp- packages.

With this goal in mind, the package will look much different.

  1. No need for a custom addQueryArgs util, because the wp- packages can just use the one from @wordpress/url, which is OK.
  2. No need for re-exporting @wordpress/api-fetch, because the wp- packages can just use that one directly, which is OK.
  3. story-editor and dashboard will not depend on any WordPress packages directly.

Edit: We're basically saying the same thing, it's just that this package will likely look quite different and contain more things. It's too early to work on this though.

@swissspidy swissspidy changed the title Packages: Move api-fetch to new structure Packages: Move api-fetch usage to new shared package Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 Nice to have Type: Enhancement New feature or improvement of an existing feature Type: Infrastructure Changes impacting testing infrastructure or build tooling
Projects
None yet
Development

No branches or pull requests

2 participants