Themes: Switch data fetching to Redux #840

ockham · 2015-11-26T11:43:41Z

Notes by @mcsf:

Context

What started as an experimental branch for a number of Redux-related changes across Themes coalesced into a substantial refactor of our data fetching components, which are now Redux-connected. This means that this PR moves Themes entirely to Redux.

Decisions

ThemesListFetcher, ActivatingTheme, and CurrentTheme (i.e. the data component) are kept as generic data components that relay application state to their children.
Selectors are given much greater importance, as they allow us to simplify the data model by avoiding forms of data duplication — notably, avoiding storage of derived data next to essential data.
Use of selectors allowed to altogether eliminate one action: SEARCH_THEMES, which was basically a virtual action (read: hack) to trigger client-side filtering of themes, a feature specific to Jetpack sites.

Stores, and helper functions in reducers are now obsolete, and are removed in Themes: Remove Flux stores #1363 to keep this PR's size down.
We need to move files to different locations in order to comply with Framework: Introduce top-level Redux state directory with sites example #751. This PR is big enough already, so that will be done separately.
As we progress, more derived data should be able to be eliminated from the global state's themes branch.
Some features of the Showcase invoke non-Themes action creators (namely, purchases and customization). This PR uses the dispatch() function from context.reduxStore in the relevant sections to dispatch the corresponding Redux actions. Another possible approach would have been to have a layer of Redux middleware that dispatches certain actions to the singleton Flux architecture.
Memoization of selectors is something very useful to have, and trivial to implement thanks to immutability. However, there are server-side-rendering-specific concerns; specifically:
- To avoid user-specific data leakage between Node requests, most memoized selectors would have to be bound to a particular request and be GC'd afterwards. This would work the same way that our global Redux store on the server is always bound to a request.
- For performance improvements, it could be very advantageous that some non-user-specific selectors were capable of cross-request memoization, though care should be given to ensure said selectors's memory usage doesn't grow unintentionally.

Bugs

Jetpack sites: searching refetches the themes list when it doesn't need to, though this only happens for the first search.
Jetpack sites: searching blurs the search field.

Testing

QA should focus around navigation of the Showcase, switching between sites, including both WP.com and Jetpack sites, searching, per-tier filtering, triggering InfiniteList's fetching, and switching between Showcase and other sections.

ockham · 2015-11-26T15:00:26Z

Just tried to stick this into #846, to provide data to the themes list we render there, server-side. This could work okayish, but ideally, I think we should use the Redux-based themes-list-fetcher on the client side too, in order not to duplicate code. So maybe that could be the goal of this PR?

ockham · 2015-11-26T15:01:58Z

@ehg @seear Join the party :-)

mcsf · 2015-11-26T17:04:37Z

I think we should use the Redux-based themes-list-fetcher on the client side too, in order not to duplicate code. So maybe that could be the goal of this PR?

Yeah, that's where I'm thinking of heading!

mcsf · 2015-11-26T17:05:52Z

#338 opened a nice precedent for client-side usage of a Redux global store, so… seems like the pieces are falling together, we can work with that momentum.

ehg · 2015-12-01T17:59:03Z

I noticed that search/tier selection was funky, 0aca329 is a possible fix :P

mcsf · 2015-12-01T19:09:34Z

I meant to fix it, thanks for taking care of it :).

ehg · 2015-12-02T10:36:45Z

client/boot/index.js

+		//		console.log( 'Flux -> Redux' );
+		//		reduxStore.dispatch( action );
+		//	} );
+		//}() );


Interesting. Did this work? :)

I'm thinking we probably don't need to integrate the two dispatchers now, as the Redux transition is getting some momentum behind it. We should check we're not relying on any 'external' actions, although from what I can remember, we're not.

Interesting. Did this work? :)

It did. :)

we probably don't need to integrate the two dispatchers now

I agree. I pushed it for the sake of communicating the exploration process, i.e. as a way of saying that this in particular had been tried.

We should check we're not relying on any 'external' actions, although from what I can remember, we're not.

Yeah, IIRC only the reverse happens, and that's something we can handle at the Redux middleware level.

ehg · 2015-12-02T10:49:02Z

shared/components/data/themes-list-fetcher/index.jsx

-		if ( page > 1 && ! loading && lastPage ) {
-			this.onLastPage && this.onLastPage();
-		}
+		fetchNextPage( site );


I got kind of confused between the AC fetchNextPage and the fetcher's fetchNextPage function. Perhaps namespace/rename?

ehg · 2015-12-02T11:10:17Z

This is looking great. I removed some dead code, and rebased.

_{The Jetpack search is quite amusing atm ;)}

mcsf · 2015-12-03T01:26:13Z

Thanks for your feedback and fixes. I just pushed substantial changes that should make this PR a lot simpler and ship-worthy. Subsequent feedback and some testing would be greatly appreciated. :)

The Jetpack search is quite amusing atm ;)

Heh, indeed. Hopefully, it's looking better now.

mcsf · 2015-12-03T01:31:34Z

On derived data and selectors: the changes brought around searchJetpackThemes/getFilteredThemes are a good illustration of what I've said in the past about Redux allowing us to significantly trim the themes branch of the global state.

Our data needs for themes are likely too simple to justify the 4 subreducers we have for it. Improving that isn't the goal of this PR, but it's something to be aware of.

ehg · 2015-12-03T11:58:14Z

the changes brought around searchJetpackThemes/getFilteredThemes are a good illustration of what I've said in the past about Redux allowing us to significantly trim the themes branch of the global state

Wow, yes. This looks much nicer. Perhaps update the PR description with what you guys have done, and how it's better? I figure this would be a good example to give to others, on how to reduxify.

mcsf · 2015-12-03T15:46:14Z

Perhaps update the PR description with what you guys have done, and how it's better? I figure this would be a good example to give to others, on how to reduxify.

Indeed. I've rewritten the PR description and added the NR label.

ockham · 2015-12-03T16:40:54Z

Quick note from testing -- purchasing a theme in single-site mode doesn't seem to update the CurrentTheme component.

ockham · 2015-12-03T17:24:16Z

How well do our flux-actions (that use my combineStores() helper at an intermediate step -- and hence Flux stores created from reducers via lib/store's createReducerStore()) work with the global Redux store (created from Redux's own createStore())? Do we have to remove them, and use our Redux actions directly everywhere?

ockham · 2015-12-03T17:30:24Z

shared/my-sites/themes/controller.js

-				)
+			// TODO: Remove function when using React 0.14
+			React.createElement( ReduxProvider, { store: context.reduxStore }, () => {
+				return React.createElement( ThemesComponent, {


Can't we shorten () => { return x; } to () => x ?

Absolutely.

mcsf · 2015-12-15T20:47:53Z

Pushed a commit to change them to // Connected props, which I'd deem fairly informal -- no need to look up the exact wording for future devs as it'd be okay to put in something equivalent, such as // connect() or whatever.

Doing pseudo-JSDoc -- introducing statements that don't exist, and would break JSDoc if we were ever going to use it -- was on second glance just so much against my OCD >:|

👍

seear · 2015-12-15T23:16:47Z

I've tested around preview->customize->activate and the Tracks events. Looking good with no apparent regressions. Noting that we don't have a working Tracks event for jetpack search, with or without this PR.

mcsf · 2015-12-16T15:56:25Z

@mcsf

Substantial refactor of our data fetching components, which are now Redux-`connect`ed. - `ThemesListFetcher`, `ActivatingTheme`, and `CurrentTheme` (i.e. the data component) are kept as generic data components that relay application state to their children. - Selectors are given much greater importance, as they allow us to simplify the data model by avoiding forms of data duplication — notably, avoiding storage of derived data next to essential data. - Use of selectors allowed to altogether eliminate one action: `SEARCH_THEMES`, which was basically a _virtual_ action (read: _hack_) to trigger client-side filtering of themes, a feature specific to Jetpack sites. An @mcsf and @ockham project, with fixes by @ehgy :-)

This way, we can avoid at least passing `getState()` as a prop to themes-selection.

…js )

Themes: Switch data fetching to Redux

ockham · 2015-12-16T19:45:44Z

shared/my-sites/themes/themes-selection.jsx

 		analytics.tracks.recordEvent( 'calypso_themeshowcase_theme_click', {
 			search_term: queryParams.search,
 			theme: theme.id,
 			results_rank: resultsRank + 1,
-			results: ThemesListStore.get(),
+			results: themesList,


Noting here that ThemesListStore.get() should have actually read ThemesListStore.getThemesList() -- we had overlooked to change get() to getThemesList() when we started using createReducerStore() to create our stores from reducers. That latter function however returns objects whose .get() method returns the entire store state. Thus, we were erroneously setting results to the contents of the entire ThemesListStore state instead (including query params and whatnot) instead of just the themes list. This has been fixed by this PR.

ockham added [Feature Group] Appearance & Themes Features related to the appearance of sites. [Status] In Progress Server Side Rendering labels Nov 26, 2015

ockham assigned ockham and mcsf and unassigned ockham Nov 26, 2015

mcsf added this to the Themes: Showcase M3-LoggedOut milestone Nov 26, 2015

ehg reviewed Dec 2, 2015
View reviewed changes

ehg force-pushed the try/themes-redux-transition branch from 0aca329 to a793461 Compare December 2, 2015 10:46

ehg reviewed Dec 2, 2015
View reviewed changes

mcsf force-pushed the try/themes-redux-transition branch from 578e4c6 to 5d45aab Compare December 3, 2015 01:23

mcsf changed the title ~~Themes: Try a pure Redux render tree~~ Themes: Switch data fetching to Redux Dec 3, 2015

mcsf added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 3, 2015

ockham reviewed Dec 3, 2015
View reviewed changes

ockham mentioned this pull request Dec 15, 2015

Themes: Refactor theme-options #1620

Closed

seear force-pushed the try/themes-redux-transition branch from f58ad6f to 523337a Compare December 15, 2015 22:06

ockham mentioned this pull request Dec 15, 2015

Themes: Remove ThemesHelpers.getSlugFromName #1628

Merged

ockham added 12 commits December 16, 2015 16:04

README: Remove theme-last-event section

6b3e56a

Data components: Inline getState()

e6a62c7

connect() main.jsx

cbc8cd6

This way, we can avoid at least passing `getState()` as a prop to themes-selection.

Upgrades: Add note on Redux

8249dd0

connect() customize/main.jsx

f7e2ff5

customize/actions: Add comment on future Reduxification

053dcc3

isJetpack(): Cast to bool (using git add shared/lib/themes/selectors.…

3ccd475

…js )

ThemesListFetcher: Add lastQuery propType

ddaeee1

Data components: Add comments on propTypes provided by connect()

85d2411

Split selectors.js into individual files

77e4cc5

Shorten propTypes comments

a84cc81

ehg force-pushed the try/themes-redux-transition branch from 523337a to a84cc81 Compare December 16, 2015 16:06

mcsf added a commit that referenced this pull request Dec 16, 2015

Merge pull request #840 from Automattic/try/themes-redux-transition

79f41f4

Themes: Switch data fetching to Redux

mcsf merged commit 79f41f4 into master Dec 16, 2015

scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 16, 2015

ehg deleted the try/themes-redux-transition branch December 16, 2015 16:15

ockham reviewed Dec 16, 2015
View reviewed changes

ajesse11x mentioned this pull request Nov 4, 2022

[Snyk] Fix for 1 vulnerabilities ajesse11x/wp-calypso#83

Open

awaisab172 mentioned this pull request Nov 5, 2022

[Snyk] Fix for 1 vulnerabilities awaisab172/wp-calypso#134

Open

ajesse11x mentioned this pull request Dec 25, 2022

[Snyk] Fix for 1 vulnerabilities ajesse11x/wp-calypso#87

Open

awaisab172 mentioned this pull request Dec 26, 2022

[Snyk] Fix for 1 vulnerabilities awaisab172/wp-calypso#141

Open

ajesse11x mentioned this pull request Nov 29, 2023

[Snyk] Fix for 20 vulnerabilities ajesse11x/wp-calypso#96

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Themes: Switch data fetching to Redux #840

Themes: Switch data fetching to Redux #840

ockham commented Nov 26, 2015

ockham commented Nov 26, 2015

ockham commented Nov 26, 2015

mcsf commented Nov 26, 2015

mcsf commented Nov 26, 2015

ehg commented Dec 1, 2015

mcsf commented Dec 1, 2015

ehg Dec 2, 2015

mcsf Dec 2, 2015

ehg Dec 2, 2015

ehg commented Dec 2, 2015

mcsf commented Dec 3, 2015

mcsf commented Dec 3, 2015

ehg commented Dec 3, 2015

mcsf commented Dec 3, 2015

ockham commented Dec 3, 2015

ockham commented Dec 3, 2015

ockham Dec 3, 2015

mcsf Dec 4, 2015

mcsf commented Dec 15, 2015

seear commented Dec 15, 2015

mcsf commented Dec 16, 2015

ockham Dec 16, 2015

Themes: Switch data fetching to Redux #840

Themes: Switch data fetching to Redux #840

Conversation

ockham commented Nov 26, 2015

Context

Decisions

Next

Bugs

Testing

ockham commented Nov 26, 2015

ockham commented Nov 26, 2015

mcsf commented Nov 26, 2015

mcsf commented Nov 26, 2015

ehg commented Dec 1, 2015

mcsf commented Dec 1, 2015

ehg Dec 2, 2015

Choose a reason for hiding this comment

mcsf Dec 2, 2015

Choose a reason for hiding this comment

ehg Dec 2, 2015

Choose a reason for hiding this comment

ehg commented Dec 2, 2015

mcsf commented Dec 3, 2015

mcsf commented Dec 3, 2015

ehg commented Dec 3, 2015

mcsf commented Dec 3, 2015

ockham commented Dec 3, 2015

ockham commented Dec 3, 2015

ockham Dec 3, 2015

Choose a reason for hiding this comment

mcsf Dec 4, 2015

Choose a reason for hiding this comment

mcsf commented Dec 15, 2015

seear commented Dec 15, 2015

mcsf commented Dec 16, 2015

ockham Dec 16, 2015

Choose a reason for hiding this comment