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

Plan for merging in the native mobile (React Native) app #11491

Closed
hypest opened this issue Nov 5, 2018 · 12 comments
Closed

Plan for merging in the native mobile (React Native) app #11491

hypest opened this issue Nov 5, 2018 · 12 comments
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) Needs Technical Feedback Needs testing from a developer perspective. [Type] Build Tooling Issues or PRs related to build tooling [Type] Overview Comprehensive, high level view of an area of focus often with multiple tracking issues

Comments

@hypest
Copy link
Contributor

hypest commented Nov 5, 2018

For some time now, the native mobile project has its own life in https://github.com/wordpress-mobile/gutenberg-mobile while still using the main Gutenberg code as a subproject (git submodule at the time of writing).

Rather recently, we had a basic integration back into the main Gutenberg repo by having the native mobile tests running as part of the main Travis jobs (#9883). Unfortunately, that effort drifted, become unmaintained and was forced to get reverted #11318.

The current state of affairs still creates extra effort to both sides, native mobile and web, to keep things moving forward but also in sync. It would be awesome if we pick up the integration again.

Let's lay a new plan.

Statement of the current problem

Currently, to develop a feature on the native mobile app, one needs to open a PR on the web repo with the necessary changes, a PR on the mobile side make sure the git submodule references are kept up to date as the PRs progress and then juggle merging them close in time. Sometimes, it's not even clear which side to merge first. In every case, after merging, PRs have to get updated to point to the ref in masters or new PRs need to open to do that part.

Proposed solution

Let's merge the two codebases and have to open single PRs.

Ideally, we'd like the teams to be able to be productive while the plan is executed. I'm thinking, a step-by-step plan that also allows some time to assess viability would be great.

Here's what I currently have in mind. Please, add your thoughts and suggestions so we can cross-check this can be made work for everyone:

Phase A (preparation step):

  1. On the mobile repo, "flatten" any external projects that currently are used as git submodules (except for the gutenberg submodule itself), either by merging their code into the mobile repo (git subtree or just git adding their code) or by transforming them into simple npm package deps.
  2. At this stage, the mobile project should compile as normal, still depending on the gutenberg submodule which will get replaced in the next phase. While at this stage, the repo is still usable and devs can work as normal.

Phase B (repo merge):

  1. Pick a folder name and git subtree the mobile repo into that.
  2. Modify the mobile build pipeline to only use the Gutenberg code in the parent folder tree, removing the need for a nested gutenberg submodule. Make sure mobile pipeline can use web's code directly to have hot-reloading enabled and working for fast turnaround times while developing
  3. Expand the Travis jobs to include the native mobile tests
    3b. Expand the integration tests so native mobile ones can run locally too
  4. Create a native mobile GitHub project and move all tickets/cards to it from https://github.com/wordpress-mobile/gutenberg-mobile/projects/1
  5. New mobile PRs should be opened in the main repo
  6. Older PRs (opened in the mobile repo) should be allowed to mature and merge in the mobile repo and then the commits pulled in the main repo. Any conflicts should be fixed with new PRs.
  7. At this stage, mobile devs should be as productive as before and better, since no PR pairs would be needed to open for a single feature as in the past.
  8. Mark https://github.com/wordpress-mobile/gutenberg-mobile as unmaintained or close it.

Phase C (deep integration):

  1. Deprecate the use of yarn in favor of npm for the mobile build pipeline
  2. Merge package.jsons to the extend the mobile app can be build from the root of the repo
  3. Set up docker images to have local testing of native compiling (Android at least, not sure if iOS can be containerized too). Alternatively, set up an external service to build the native app or library.

Phase D (clients)

  1. The only known client application this far will be the official WordPress mobile apps https://apps.wordpress.org. We're in the process of laying down the ground works for that integration and we'll continue to iterate.

Feedback request

What do you think? Anything missing? Anything going down in the wrong direction? Any adjustments to make to the plan or the steps? Any thoughts on having the native mobile code as a npm package? How would that work? Any different plans?

As it stands, Phases C, D are the most "hazy" at the moment and would like to ask for help and ideas about expanding them.

Pinging some people here in particular to have their thoughts early on: @mtias , @youknowriad , @gziolo, @koke and CC @Tug to keep in the loop early on too.

@hypest hypest added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Nov 5, 2018
@gziolo
Copy link
Member

gziolo commented Nov 5, 2018

I was wondering if we could speed it up by trying to integrate a scaffolded RN project into Gutenberg and then only repeat steps for Gutenberg mobile. To do so you would need to do the following

git clone git@github.com:WordPress/gutenberg.git
cd gutenberg
npm install -g react-native-cli
react-native init mobile

and try to set this project as a local private npm package controlled by Lerna.

@hypest
Copy link
Contributor Author

hypest commented Nov 5, 2018

react-native init mobile

Thanks @gziolo . BTW, that's what the current mobile repo and app is structured on. Besides, that scaffolded project will have zero integration with Gutenberg so, not sure what to add to it next to try the integration.

That said,

controlled by Lerna.

I have zero experience with Lerna so, can you expand what that will give us? Maybe we can do it directly to a a subtree'd mobile repo?

@gziolo
Copy link
Member

gziolo commented Nov 6, 2018

Opened #11533 with a proof of concept, I left my comments there.

@Tug
Copy link
Contributor

Tug commented Nov 6, 2018

Sounds like a very sane plan 👍

I think preparation could have a few more steps though to make the merge process a little bit easier.

We could start adopting gutenberg coding guidelines first, meaning:

  • Dropping Flow
  • Refactor the components. For instance not call bind inside render() but instead bind functions in the constructor as it's done in gutenberg.

We could also switch to using the gutenberg store in gutenberg-mobile.
Or use npm instead of yarn before the merge.

@gziolo
Copy link
Member

gziolo commented Nov 6, 2018

Some things I did in #11533:

  • I dropped Flow
  • I dropped yarn

Something I could do:

  • Use @wordpress/element instead of react directly

Things we have to do later:

  • Ensure all RN dependencies are listed in packages (react-native, react-native-svg, etc.).

@mtias mtias added [Type] Overview Comprehensive, high level view of an area of focus often with multiple tracking issues [Type] Build Tooling Issues or PRs related to build tooling labels Nov 6, 2018
@hypest
Copy link
Contributor Author

hypest commented Nov 8, 2018

I dropped Flow

Is that a hard requirement on Gutenberg-web's side? Like, does the pipeline break if Flow is enabled on the mobile code?

@gziolo
Copy link
Member

gziolo commented Nov 8, 2018

Is that a hard requirement on Gutenberg-web's side? Like, does the pipeline break if Flow is enabled on the mobile code?

I don't know if it breaks, but it would be nice to drop it for consistency.

@hypest
Copy link
Contributor Author

hypest commented Nov 8, 2018

I don't know if it breaks

Ok, cool.

but it would be nice to drop it for consistency

In principal yes and makes sense. In practice, as the mobile side team still tries to gain experience in the JS/RN and the ecosystem and RN projects seem to favor Flow and even Typescript, I'd prefer it if we try and keep it in for now, especially if it doesn't break anything.

@gziolo gziolo added the Needs Decision Needs a decision to be actionable or relevant label Apr 23, 2019
@gziolo
Copy link
Member

gziolo commented Apr 23, 2019

@hypest and @koke - is is still going to happen? What's the current status.

@hypest
Copy link
Contributor Author

hypest commented Apr 30, 2019

@hypest and @koke - is is still going to happen? What's the current status.

👋 @gziolo . Yes, there is still the need to remove the chore of having 2 PRs opened for most of the work on mobile so, we should at least try to implement Phase A and B from the proposal on this ticket.

My impression is that we still need feedback from the web side of things on this plan. The attempt to dive directly to fully-merged projects was interesting but not usable for assessing Phase A and Phase B and this ticket overall.

So, the status of this ticket is: needs feedback from the web teams. Let's keep the ticket open for that reason.

To help make it more specific and hopefully more actionable, the proposal here can be simplified to: let's merge the repos but not the projects just yet and please, provide feedback on the steps on the ticket or share other proposals on how to accomplish that.

@gziolo gziolo added Needs Technical Feedback Needs testing from a developer perspective. and removed Needs Decision Needs a decision to be actionable or relevant labels May 23, 2019
@gziolo
Copy link
Member

gziolo commented Oct 10, 2019

@Tug did some experiments in #17456. Do you have any feedback to share?

@gziolo
Copy link
Member

gziolo commented Jul 2, 2020

It has happened already 🎉

If there’s anything left, let’s file individual more targeted issues. Great team work 👏

@gziolo gziolo closed this as completed Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) Needs Technical Feedback Needs testing from a developer perspective. [Type] Build Tooling Issues or PRs related to build tooling [Type] Overview Comprehensive, high level view of an area of focus often with multiple tracking issues
Projects
None yet
Development

No branches or pull requests

4 participants