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

Subscribers store: convert generators to thunks #98333

Merged
merged 1 commit into from
Jan 15, 2025
Merged

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented Jan 14, 2025

Converts the Subscribers store actions from generators to thunks. That's the modern and easier-to-understand way to do actions in data stores.

How to test:
Go to the /subscribers/:site page in Calypso and in Jetpack Cloud and verify that the subsciber import flows continue to work correctly.

@jsnajdr jsnajdr requested review from simison, tyxla and a team January 14, 2025 12:16
@jsnajdr jsnajdr self-assigned this Jan 14, 2025
@jsnajdr jsnajdr requested a review from a team as a code owner January 14, 2025 12:16
@matticbot matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 14, 2025
@jsnajdr jsnajdr force-pushed the thunkify/subscribers branch from cbcfef0 to 22f83a5 Compare January 14, 2025 12:27
@matticbot
Copy link
Contributor

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • notifications
  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug thunkify/subscribers on your sandbox.

@matticbot
Copy link
Contributor

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~91 bytes removed 📉 [gzipped])

name         parsed_size           gzip_size
subscribers       +159 B  (+0.0%)      -17 B  (-0.0%)
people            +159 B  (+0.0%)      -37 B  (-0.0%)
import            +111 B  (+0.0%)      -37 B  (-0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Member

@allilevine allilevine left a comment

Choose a reason for hiding this comment

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

I tested:

  • Manual import in both Calypso Blue and Green.
  • CSV import in both Calypo Blue and Green.
  • Manual and CSV imports with categories in Calypso Blue.
  • Substack subscriber import.

@jsnajdr
Copy link
Member Author

jsnajdr commented Jan 14, 2025

Thanks for the review @allilevine 👍 Do you happen to know if the Subcribers store is used anywhere outside Calypso? What about other stores in the data-stores packages? I have ideas for some potential changes and would like to know how breaking they can be.

@simison
Copy link
Member

simison commented Jan 14, 2025

Subscribers store AFAIK is used just in Calypso blue & Jetpack Cloud; the only other place would be Jetpack plugins.

@allilevine
Copy link
Member

Thanks for the review @allilevine 👍 Do you happen to know if the Subcribers store is used anywhere outside Calypso? What about other stores in the data-stores packages? I have ideas for some potential changes and would like to know how breaking they can be.

@jsnajdr I'm not aware of any Subscriber store uses outside Calypso, as Mikael commented. It looks like it was intended to be consumed outside of Calypso, but I haven't seen any evidence that it was: pcmemI-Jw-p2#comment-901

I worked on packages/data-stores/src/newsletter-categories/index.ts recently and that's also used in Calypso. The README says "It is meant to be helpful for projects developed inside the Calypso monorepo."

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the cleanup @jsnajdr!

@jsnajdr jsnajdr merged commit 36dec1a into trunk Jan 15, 2025
13 checks passed
@jsnajdr jsnajdr deleted the thunkify/subscribers branch January 15, 2025 08:14
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants