-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Proposal: Consolidate Gutenberg initialProps handling #12444
Proposal: Consolidate Gutenberg initialProps handling #12444
Conversation
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
c14261c
to
b17bf53
Compare
You can test the changes on this Pull Request by downloading the APK here. |
Thanks @mchowning for the detailed proposal! I've had similar thoughts about muxing to reduce the arity of these very interfaces 😄 , and I think you've outlined the essential requirements very well in your description.
I welcome a reduction here 😃, and I think the greatest value from this is that it can reduce the cognitive load while reasoning about the collaborating classes:
i.e. making these interfaces more readable will also improve the experience of maintaining other nearby areas of the code base. I especially agree with these points:
and
I wonder whether we could reduce the boiler plate even more, reducing repetition in the builder class.. though I'm not sure that would be possible without losing some compile-time guarantees 🤔 |
Thanks for giving this some thought @mkevins !
We actually may not lose as many compile-time guarantees as you suspect because even using the builder object, I'm losing some compile-time guarantees because I'm using We could drop the A second reason I forgot to mention for why I used the We could switch |
Thanks for further elaborating.
I wonder about this.. are we actually losing any guarantees? I still think (in the current state of your proposal) we get all the same guarantees we had before, and this runtime non-null assertion is "extra". I mean, at least, in this proposal, I guess a compilation failure will still clearly signal the bridge incompatibility when a member / parameter / prop is added in one place but not the other. Is there a scenario where we currently get compile errors, in which the proposed implementation will instead have runtime errors? Regarding the mutable / immutable, I like the current approach (immutable), but don't have strong feelings either way. |
I do think we lose a small bit of compile-time safety around the Personally, I think this tradeoff is more than worth it with what we gain in reduced boilerplate and the clarity the |
Ah, right. There is no signal to add the necessary mutations prior to invoking |
Good point, I was able to move |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are a huge improvement, well done! The thread with @mkevins was enlightening as well. I agree with the conclusions that it is worth it to have the immutable GutenbergProps
defined in the react-native-bridge
and a separate GutenbergPropsBuilder
class in WPAndroid. Improvements here made me think about how we may write up steps for integrating GB-Mobile in third party apps in the future - changes here will definitely be helpful for that future work.
I left a comment on the Gutenberg PR about updating the demo app, but otherwise I think these changes are in pretty good shape. Sanity tests running WPAndroid all looked good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for the improvement here!
c57a73e
to
8467702
Compare
8467702
to
7ac9672
Compare
The purpose of this PR is to simplify the handling of passing initial props to the Gutenberg editor. Because this is a bit of an architecture change, I think it would be helpful to get feedback from more than one reviewer if possible. 🙇
Related PRs:
Current State
Currently, the process when you need to pass a new initialProp is you figure out where you have access to that prop, and then you manually pass it up through the relevant
Activity
andFragments
until you get it to theWPAndroidGlueCode
where you insert the value into the initial props (i.e.,EditPostActivity
→GutenbergEditorFragment
→GutenbergContainerFragment
→WPAndroidGlueCode
). As a result, (1) adding a new initial prop means we have to pass that prop through all of those different classes, which results in a lot of boilerplate (see specifics below⤵); and (2) in order to track down whether a specific object is being passed for inclusion in the initial props we have to trace it's path through all those classes until we get toWPAndroidGlueCode
where it is inserted into the initial props.Steps Involved in Adding an Initial Prop Under Current State
To make this more concrete, adding an object for inclusion as a new initial prop under the current approach requires the following:
Frequently, the prop is available in
EditPostActivity
so we add a new parameter to theGutenbergEditorFragment::newInstance
so that we can pass the value to theGutenbergEditorFragment
. This method currently has 17 parameters, so this would increase it to 18! 🙃Inside
GutenbergEditorFragment::newInstance
we have to create a String key for the new parameter and store that parameter in the fragment's argument bundle.In
GutenbergEditorFragment::onCreate
we pull the new object out of the fragment's argument bundle, create a new parameter in theGutenbergContainerFragment::newInstance
method for that object and pass it.Then we create another
String
key for the new parameter inGutengergContainerFragment
and put the parameter into that Fragment's argument bundle.Now we pull the object out of
GutenbergContainerFragment
's argument bundle, add a new parameter toWPAndroidGlueCode
for that object, and pass it toWPAndroidGlueCode::onCreateView
, which currently accepts 17 parameters.In
WPAndroidGlueCode::onCreateView
, we finally take the object and insert into theinitialProps
object that is passed to React Native.Proposed Change
In this PR, I have created a
GutenbergProps
data class in `react-native-gutenberg-bridge. This class's purpose is to (1) define the information that is required for Gutenberg's initial props, and (2) construct that initial props object.Using this class by itself would not significantly simplify things on the WPAndroid side, however, because we get the data needed for the initial props from different locations (sometimes
EditPostActivity
, sometimesGutenbergEditorFragment
, and sometimesGutenbergContainerFragment
). That means that the only time we have all the data we need to construct this object is at the end of that chain. It would be much better if we could instead construct this single object inEditPostActivity
and pass it down through the various fragments, adding the needed information along the way (instead of passing all the information separately, as we do now).The editor module defines a
GutenbergPropsBuilder
object to facilitate exactly that. I first construct an instance ofGutenbergPropsBuilder
inEditPostActivity
and then pass that single object through all the steps discussed above, adding information along the way (for example). Then we just callGutenbergPropsBuilder.build()
to create a fully-realizedGutenbergProps
object that we can pass toWPAndroidGlueCode
.This required adding 1 new method parameter to pass the new
GutenbergPropsBuilder
instance toGutenbergEditorFragment::newInstance
,GutenbergContainerFragment::newInstance
andWPAndroidGlueCode::onCreateView
. This enables us to remove a total of 24 parameters from those same method calls though 😃 because those parameters are now stored in theGutenbergPropsBuilder
instance that is being passed.Steps Involved in Adding an Initial Prop Under Proposed Change
Have updated these steps to reflect improvements made after the discussion in this PR. Old text was left, but
struckthrough. Discussion in the comments on this PR will provide additional context.Add the new initial prop to the
GutenbergProps
object, and take the new information as a construction parameter.This will create a compile error in the
GutenbergPropsBuilder
's construction of theGutenbergProps
object, so we need to fix that, which willprobably involve enabling theinvolve adding the additional information as a parameter to eitherGutenbergPropsBuilder
to accept additional information needed to construct theGutenbergProps
instance (either via a construction parameter or through a setter method)GutenbergPropsBuilder
's constructor or it'sbuild(...)
method. The decision of which method should be changed should depend on where it is easier to provide the required information.At the point where the new information is available (regardless of whether that is inThat will create a new compile error at the point where the constructor or build method is called, so we will need to pass the required information in that method.EditPostActivity
,GutenbergEditorFragment
, or theGutenbergContainerFragment
) pass the information to theGutenbergPropsBuilder
instance.Possible Issues With Proposed Change
Adding or Removing a Prop Requires Updating the WPAndroid and Gutenberg repos
One possible objection to this change is that the
GutenbergProps
object is defined in the@wordpress/react-native-bridge
module, which is in theGutenberg
repo, which means that when you want to, for example, add a new initial prop, you have to (1) update theGutenbergProps
object inGutenberg
, (2) update the gb-mobile submodule ref, and (3) update WPAndroid to provide the newly required prop.It seems easier if we just define the
GutenbergProps
object inside the editor module inWPAndroid
and have it construct and pass an initial propsBundle
to Gutenberg'sWPAndroidGlueCode
class. That way we would only have to update code in the WPANdroid repo to add or remove initial props on the native side (i.e., we wouldn't have to change any native code in Gutenberg). Although this would definitely be a nice benefit, I am inclined against it for two reasons:Doing it this way means that the
WPAndroid
repo is defining what initial props need to be passed to theWPAndroidGlueCode
. I think that it makes far more sense forWPAndroidGlueCode
to have the knowledge about what initial props React Native needs and to for it to "tell" WPAndroid what is required. For example, if another app ended up using gutenberg-mobile, I don't think they should have to look at either javascript orWPAndroid
to see what props are expected from native; the Android code inreact-native-gutenberg-bridge
should have that information in the type system (in this PR theGutenbergProps
class serves that purpose).I think that it will actually be pretty rare that we will ever add or remove an initial prop without also needing to make JS change to Gutenberg, so I doubt that we would be able to take advantage of "only" having to update WPAndroid very often in practice.
We Have Separate
GutenbergProps
andGutenbergPropsBuilder
classesA similar change we could make would be to not make the⤴️ , I like having the immutable
GutenbergProps
object immutable, which would mean we wouldn't need the (mutable)GutenbergPropsBuilder
object to accumulate the relevant information as it is passed up fromEditPostActivity
. Similar to what I said aboveGutenbergProps
object as a clear representation of what Gutenberg needs for its initial props (if you can construct theGutenergProps
object, then you know you have all the information that React Native needs). The fact that within WPAndroid all the information needed for the initial props is not conveniently available from a single location is an implementation detail of WPAndroid and (imo) thereact-native-gutenberg-bridge
module should not "know" about that implementation detail or attempt to work around it.Feedback Please!
Although I feel pretty good about what I'm proposing here, I still consider this a proposal, so please share any feedback, suggestions, or better alternatives you might have. 🙇
To Test
Smoke test the Gutenberg editor to ensure that the initial props are getting properly set and it is functioning normally. This PR should not result in any changes in how the app behaves.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.