Use Gutenberg store #226

Tug · 2018-11-07T14:09:16Z

This PR switches the current store with the one provided by gutenberg.

Only App and AppContainer are updated for now since those deal with the state. Some minor changes on BlockManager are necessary since the gb store already deals with merging blocks.

We'll also want to connect some of our components to the store and reuse existing components from the gutenberg repository (BlockList, BlockListBlock and BlockEdit)

Testing Instructions

git submodule update --init
yarn install
yarn start:reset
Check that there are no important regressions on android and ios (yarn ios and yarn android)
Run the tests bash ./.travis/travis-checks-js.sh

Tug · 2018-11-07T17:02:02Z

src/block-management/block-manager.js

@@ -115,12 +117,14 @@ export default class BlockManager extends React.Component<PropsType, StateType>
 	}

 	static getDerivedStateFromProps( props: PropsType, state: StateType ) {
-		if ( props.fullparse === true ) {
+		if ( state.blocks !== props.blocks ) {


This might impact performance as the list of blocks will be recreated on any block change. I'll see if we can do something about it.

Tug · 2018-11-07T17:08:58Z

src/app/AppContainer.js

+	let blocks = state.blocks;
+	let refresh = !! ownProps.refresh;
+
+	const newBlocks = ownProps.blocks.map( ( block ) => {


This is a hack to set the focus property on each block. Once we use the isSelected() selector in our components we can simplify this

Tug · 2018-11-07T17:11:43Z

src/block-management/block-manager.js

@@ -263,8 +250,8 @@ export default class BlockManager extends React.Component<PropsType, StateType>
 			list = (
 				<FlatList
 					style={ styles.list }
-					data={ this.props.blocks }
-					extraData={ this.props.refresh, this.state.inspectBlocks }


Not sure how it was working before but this looks like it's equivalent with extraData={ this.state.inspectBlocks }

hypest · 2018-11-07T22:44:38Z

src/app/AppContainer.js

+
+const mapStateToProps = ( state, ownProps ) => {
+	let blocks = state.blocks;
+	let refresh = !! ownProps.refresh;


👋 @Tug , I'm trying to figure out how refresh ends up being a field in ownProps, and I can't find it.

At his point in the code, I understand that ownProps is the props passed to the AppContainer component. The two HOCs composed over it (withDispatch and withSelect) don't seem to add the refresh field.

Can you point me to where this refresh is added/set? Thanks!

It's given as props to BlockManager a few lines below https://github.com/wordpress-mobile/gutenberg-mobile/pull/226/files#diff-0b657b049f5ed3fc0862ff8bf5c9e4d4R29

Then to FlatList here https://github.com/wordpress-mobile/gutenberg-mobile/pull/226/files#diff-2d50daf729e068ecebcce113d1ce7f9aR254

Hmm, let me see I understand this. mapStateToProps will be called for every component down the line so, BlockManager will get the refresh prop from the mapStateToProps() call on the component above BlockManager?

Plus, ownProps.refresh will be undefined for AppContainer?

Not really for every component, connect is just an HOC that will re-render the inside component (MainApp in this case) on state change (meaning it will bind listeners on this.props.store given by the provider in App).
So every properties returned in mapStateToProps are added to the props of MainApp and as you can see in MainApp.js https://github.com/wordpress-mobile/gutenberg-mobile/blob/master/src/app/MainApp.js#L33 this component passes all its props down to BlockManager.
In this case refresh is a hack for FlatList, we're just flipping its value whenever we detect a change in blocks. Later we'll probably want to remove it and rely on blocks only.

So every properties returned in mapStateToProps are added to the props of MainApp and as you can see in MainApp.js

Cool, that's what I've understood so far, thanks. So, I'm back to the original question: where is the value of ownProps.refresh set? My impression is that it's always undefined since AppContainer's props never get augmented with refresh. Right?

You are right, it actually isn't set.
Yeah we should just be using false instead.

hypest · 2018-11-08T08:33:11Z

Had a look @Tug and I tried to follow the code paths and data flow. This is not exactly a review pass but more like a check to see if I have understood things. Here's what I got:

The "old" store is there but the "old" reducer is essentially vestigial now, it's not used
There is no new reducer atm, its functionality is now embedded in the mapDispatchToProps, right? Examples: the parseBlocksAction and serializeToNativeAction mappings.
mapStateToProps is essentially bringing in the blocks list from the data package
on Android, animations no longer seem to work

Can you verify that those "findings" above are accurate? Thanks!

Tug · 2018-11-08T10:43:39Z

The "old" store is there but the "old" reducer is essentially vestigial now, it's not used

Yes. We're not completely removing the store in this PR as we're still using setupStore and reusing the data. Also it will require a bit more changes, I think Types will be affected as well.

There is no new reducer atm, its functionality is now embedded in the mapDispatchToProps, right? Examples: the parseBlocksAction and serializeToNativeAction mappings.

There is. We're now using the gutenberg editor reducer with this change (See all the selectors and actions we're getting from select( 'core/editor' ) and dispatch( 'core/editor' )).
And serializeToNativeAction is a bit of an exception as it doesn't change the store, it just reads from it.

mapStateToProps is essentially bringing in the blocks list from the data package

Yeah mapStateToProps was simply dumping the whole store state into the MainApp props. Now we're injecting the blocks prop from the gutenberg store using withSelect and picking it (using ownProps.blocks) in mapDispatchToProps instead of state.blocks

on Android, animations no longer seem to work

I haven't noticed but I'll investigate 👍

hypest · 2018-11-08T12:01:52Z

Thanks for confirming @Tug !

On a different thing now: Since the actions are dispatched to the GB store, how is the old store changed and the change propagating down? Apparently it's propagated but, trying to connect the dots there. Any pointers?

Tug · 2018-11-08T15:41:43Z

It's not propagated anymore, I removed the dispatch() calls in e4826ec

So basically the gb-mobile store is just initialized with the data from initial-html.js then we retrieve the blocks to initialize the gb store and we don't read or write from the gb-mobile store ever again.

Tug · 2018-11-08T16:08:39Z

src/block-management/block-holder.js

-export default class BlockHolder extends React.Component<PropsType, StateType> {
-	constructor( props: PropsType ) {
-		super( props );
-		this.state = {


this state isn't used anywhere afaics.

hypest · 2018-11-09T13:03:57Z

package.json

@@ -13,6 +13,7 @@
    "@babel/plugin-transform-runtime": "^7.1.0",
    "@wordpress/babel-preset-default": "^1.1.2",
    "@wordpress/block-serialization-spec-parser": "^1.0.0",
+    "@wordpress/scripts": "^2.4.0",


Not sure what is wrong with including @wordpress/scripts as a dep (yarn install fails with error Package "" refers to a non-existing file '"<path to project root>/babel-preset-default"') but, do we really need to add this dep? Building and running the app seems fine even without it.

hypest · 2018-11-12T11:24:19Z

package.json

@@ -81,6 +81,7 @@
  },
  "dependencies": {
    "@babel/runtime": "^7.0.0",
+    "@wordpress/jest-preset-default": "^3.0.2",


Can we just upgrade the devDependencies one or do we indeed need a runtime dep here?

Sounds about right 👍

Tug · 2018-11-13T14:34:28Z

This PR requires the following gutenberg patch WordPress/gutenberg#11777.
An alternative approach could be taken so we don't get blocked by this issue.

…nberg tests run

…events

…h dataSource and use react lifecycle methods to update instead

…ymore

hypest

Looks good to me!

Thanks for the hard work and iteration on this one @Tug, feel free to merge!

Tug · 2018-11-15T09:02:07Z

Thanks for those excellent review @hypest! You spotted all the mistake I did here :)

Tug added [Type] Enhancement Improves a current area of the editor [Status] Not Ready For Review labels Nov 7, 2018

Tug self-assigned this Nov 7, 2018

Tug requested a review from hypest November 7, 2018 14:09

Tug force-pushed the add/gb-store branch 4 times, most recently from 31926c6 to e4826ec Compare November 7, 2018 16:20

Tug commented Nov 7, 2018

View reviewed changes

Tug force-pushed the add/gb-store branch from 1a2b13c to cea1e9c Compare November 7, 2018 17:05

Tug commented Nov 7, 2018

View reviewed changes

hypest reviewed Nov 7, 2018

View reviewed changes

hypest mentioned this pull request Nov 8, 2018

Detect content changed #225

Merged

Tug force-pushed the add/gb-store branch from cea1e9c to 4e69b51 Compare November 8, 2018 10:42

Tug commented Nov 8, 2018

View reviewed changes

Tug removed the [Status] Not Ready For Review label Nov 8, 2018

Tug force-pushed the add/gb-store branch from 9b7802a to 4434c8e Compare November 9, 2018 12:01

hypest reviewed Nov 9, 2018

View reviewed changes

Tug force-pushed the add/gb-store branch 4 times, most recently from fad9a01 to 37faa04 Compare November 12, 2018 09:56

hypest reviewed Nov 12, 2018

View reviewed changes

Tug force-pushed the add/gb-store branch from a0acd81 to c6d1998 Compare November 12, 2018 12:55

Tug mentioned this pull request Nov 13, 2018

Experiment with reusing the gutenberg store in the block manager #215

Closed

Tug force-pushed the add/gb-store branch 3 times, most recently from 4b546df to 1eca84c Compare November 13, 2018 14:19

Tug force-pushed the add/gb-store branch 4 times, most recently from f274656 to d8c9850 Compare November 13, 2018 19:32

Tug added 8 commits November 13, 2018 20:32

Connect App and AppContainer to the gutenberg store as well

595bd4f

Replace local store with gutenberg store

ab76265

Cleanup and optimize blocks change

d95d6a2

Mock react-native-recyclerview-list

4812743

Intialize refresh to false by default

f4c3bc0

Add jest-preset-default as dependency to ensure it's loaded when gute…

3f3787c

…nberg tests run

Do not overwrite dataSource but instead keep it in sync using the ui …

7ce9175

…events

More reliable way to update focus, do not share object references wit…

afe12a2

…h dataSource and use react lifecycle methods to update instead

Tug force-pushed the add/gb-store branch from d8c9850 to f4806da Compare November 13, 2018 19:40

Tug added 5 commits November 14, 2018 09:54

Refactor App and AppContainer to not depend on the gb-mobile store an…

0357cfe

…ymore

Remove old store

4ad24dc

Fix merging blocks using the new store

ec35d6a

Add back the test that modifies the store

34bc0b9

Make a shallow copy of blocks instead of deep copy

0c614ea

Tug force-pushed the add/gb-store branch from f4806da to 0c614ea Compare November 14, 2018 08:54

Fix updating dataSource from blocks props

d7ea973

hypest approved these changes Nov 15, 2018

View reviewed changes

Tug merged commit 5c2e876 into master Nov 15, 2018

Tug deleted the add/gb-store branch November 15, 2018 08:58

Tug mentioned this pull request Nov 15, 2018

Send changed to the native bridge if html changed since last time #242

Merged

etoledom mentioned this pull request Nov 15, 2018

Insert block button adds duplicate blocks once switch to html view and back #233

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Gutenberg store #226

Use Gutenberg store #226

Tug commented Nov 7, 2018 •

edited

Tug Nov 7, 2018 •

edited

Tug Nov 7, 2018

Tug Nov 7, 2018

hypest Nov 7, 2018 •

edited

Tug Nov 7, 2018

hypest Nov 7, 2018

Tug Nov 8, 2018 •

edited

hypest Nov 8, 2018

Tug Nov 9, 2018

hypest commented Nov 8, 2018

Tug commented Nov 8, 2018 •

edited

hypest commented Nov 8, 2018

Tug commented Nov 8, 2018 •

edited

Tug Nov 8, 2018

hypest Nov 9, 2018

hypest Nov 12, 2018 •

edited

Tug Nov 13, 2018

Tug commented Nov 13, 2018 •

edited

hypest left a comment

Tug commented Nov 15, 2018

Use Gutenberg store #226

Use Gutenberg store #226

Conversation

Tug commented Nov 7, 2018 • edited

Testing Instructions

Tug Nov 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypest Nov 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tug Nov 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypest commented Nov 8, 2018

Tug commented Nov 8, 2018 • edited

hypest commented Nov 8, 2018

Tug commented Nov 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypest Nov 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tug commented Nov 13, 2018 • edited

hypest left a comment

Choose a reason for hiding this comment

Tug commented Nov 15, 2018

Tug commented Nov 7, 2018 •

edited

Tug Nov 7, 2018 •

edited

hypest Nov 7, 2018 •

edited

Tug Nov 8, 2018 •

edited

Tug commented Nov 8, 2018 •

edited

Tug commented Nov 8, 2018 •

edited

hypest Nov 12, 2018 •

edited

Tug commented Nov 13, 2018 •

edited