Skip to content

Conversation

@tyxla
Copy link
Member

@tyxla tyxla commented Mar 14, 2023

Proposed Changes

This PR removes the unused Reader store. It hasn't been in use since #54196 landed.

Part of #74399. A follow-up to #73890.

Testing Instructions

  • Verify that the removed store is not in use.
  • All checks should be green and Calypso should build well.

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-ajp-p2)?

@tyxla tyxla added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] Reader The reader site on Calypso. [Type] Janitorial Data-Store labels Mar 14, 2023
@tyxla tyxla requested review from a team, p-jackson and yansern March 14, 2023 14:16
@tyxla tyxla self-assigned this Mar 14, 2023
@matticbot
Copy link
Contributor

matticbot commented Mar 14, 2023

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Contributor

@flootr flootr left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@p-jackson
Copy link
Member

I wonder if this could be useful to keep around for the next time someone wants to gate something behind an "is a11n" flag and they're not in the Calypso client codebase. The original reason the store was added was so Gutenboarding had access to the reader team data to implement such a feature gate. But a future example might be an ETK feature. It can't import Calypso's redux store, but it can import @automattic/data-stores.

But maybe that's a moot point because there are no new ETK features, only Jetpack features 🤷

@tyxla
Copy link
Member Author

tyxla commented Mar 15, 2023

No unused code is useful to keep IMHO 😉 We can always bring it back if necessary - work is never lost in history 😉

Copy link
Contributor

@mashikag mashikag left a comment

Choose a reason for hiding this comment

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

FYI, I have the following tasks created that will be implemented within the next 2 weeks. The plan is to introduce the new queries into the reader data-store.

However, as of now I have no clear plans for introducing any new reader redux-store state. On the other hand, there are more queries to come in the near future, I just did not create tasks for them yet.

At the same time, I see no need for any of the store state being removed in this PR, therefore I am approving it.

@tyxla tyxla merged commit 4d59f82 into trunk Mar 15, 2023
@tyxla tyxla deleted the remove/unused-reader-data-store branch March 15, 2023 10:48
@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 Mar 15, 2023
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.

6 participants