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

[Gutenberg] Enable build from monorepo #11956

Conversation

cameronvoell
Copy link
Contributor

@cameronvoell cameronvoell commented May 20, 2020

Description

This PR adds the necessary changes for building WordPress-Android using the new Gutenberg/Gutenberg-Mobile Monorepo setup.

Related PR's:

Gutenberg Monorepo PR's for fixing WPAndroid build

Background Info on build changes to stop using jitpack for building GB-Mobile

To test:

cd libs/gutenberg-mobile
npm install
npm run wpandroid

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 20, 2020

Warnings
⚠️ PR is missing at least one label.
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 20, 2020

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@cameronvoell cameronvoell requested review from Tug and ceyhun May 20, 2020 05:30
Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

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

👋 @cameronvoell ! Lots of great progress here!

Not sure if this has been implemented yet, but building with the wp.BUILD_GUTENBERG_FROM_SOURCE flag set to true fails for me:

Could not determine the dependencies of task ':react-native-video:generateDebugRFile'.
> Could not resolve all task dependencies for configuration ':react-native-video:debugRuntimeClasspath'.
   > Could not find com.facebook.react:react-native:0.61.5.
     Searched in the following locations:
       - file:/Users/matt/.m2/repository/com/facebook/react/react-native/0.61.5/react-native-0.61.5.pom
       - file:/Users/matt/.m2/repository/com/facebook/react/react-native/0.61.5/react-native-0.61.5.jar
       - file:/Users/matt/dev/a8c/WordPress-Android/libs/gutenberg-mobile/gutenberg/node_modules/react-native-video/node_modules/react-native/android/com/facebook/react/react-native/0.61.5/react-native-0.61.5.pom
       - file:/Users/matt/dev/a8c/WordPress-Android/libs/gutenberg-mobile/gutenberg/node_modules/react-native-video/node_modules/react-native/android/com/facebook/react/react-native/0.61.5/react-native-0.61.5.jar
     Required by:
         project :react-native-video

Let me know if you think that might just be a local issue on my machine.

Also, if I am not building from source (wp.BUILD_GUTENBERG_FROM_SOURCE=false), I'm getting the error mentioned in this comment when I load the editor. Resolved

@@ -9,25 +9,27 @@ include ':libs:login:WordPressLoginFlow'
include ':WordPressMocks'
project(':WordPressMocks').projectDir = new File(rootProject.projectDir, properties.getOrDefault('wp.wordpress_mocks_path', 'libs/mocks') + '/WordPressMocks')

include ':react-native-aztec'
project(':react-native-aztec').projectDir = new File(rootProject.projectDir, 'libs/gutenberg-mobile/react-native-aztec/android')
include ':@wordpress_react-native-aztec'
Copy link
Contributor

@mchowning mchowning Jun 1, 2020

Choose a reason for hiding this comment

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

I'm curious, what is the significance of using an "@" and adding the "wordpress" prefix both here and for @wordPress_react-native-bridge?

Copy link
Contributor

Choose a reason for hiding this comment

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

It just reflects the npm package name @wordpress/react-native-aztec. When you autolink a react-native dependency, it will keep the @ and replace the / with an _.

@cameronvoell cameronvoell changed the title Post GB-Mobile Monorepo build fixes [Gutenberg] Enable build from monorepo Jun 4, 2020
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jun 5, 2020

You can test the changes on this Pull Request by downloading the APK here.

@hypest
Copy link
Contributor

hypest commented Jun 17, 2020

Went in to try to help with the Could not find com.facebook.react:react-native:0.61.5 issue @mchowning mentioned above and it seems that this diff on the root build.gradle would suffice:

-url "$rootDir/libs/gutenberg-mobile/node_modules/react-native/android"
+url "$rootDir/libs/gutenberg-mobile/gutenberg/node_modules/react-native/android"

essentially just pointing out that the node_modules we're interested in now is nested inside the gutenberg folder.

@cameronvoell cameronvoell marked this pull request as ready for review June 18, 2020 17:04
@cameronvoell
Copy link
Contributor Author

There are two items I could use feedback on this PR @mchowning @marecar3

  1. wp.BUILD_GUTENBERG_FROM_SOURCE = true, could you check to see if editing an unsupported block works for you? It's been working for me, but @hypest was unable to have that work this today

  2. wp.BUILD_GUTENBERG_FROM_SOURCE = 'false':
    I'm currently seeing an error running installWasabiDebug related to our patch-react-native script. If I remove the call to the patch script from our post-install in gutenberg-mobile/package.json, the error goes away. Any ideas on if we need this patch in this build flow, or how to apply it correctly are welcome. See error log below:

> [email protected] patch-react-native /Users/cameronvoell/FRESH/WordPress-Android/libs/gutenberg-mobile
> git apply --ignore-whitespace patches/react-native+0.61.5.patch

error: patch failed: gutenberg/node_modules/react-native/Libraries/ActionSheetIOS/RCTActionSheetManager.m:66
error: gutenberg/node_modules/react-native/Libraries/ActionSheetIOS/RCTActionSheetManager.m: patch does not apply
error: patch failed: gutenberg/node_modules/react-native/React/Views/RCTShadowView.m:156
error: gutenberg/node_modules/react-native/React/Views/RCTShadowView.m: patch does not apply
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] patch-react-native: `git apply --ignore-whitespace patches/react-native+0.61.5.patch`
npm ERR! Exit status 1

Changes below fix the error, and I'm able to open the editor, but I'm assuming this is here for a reason, and didn't get to dig in yet on whether we need to apply the patch in this specific flow:
gutenberg-mobile/package.json

   "scripts": {
-    "postinstall": "npm install --prefix gutenberg && npm run patch-react-native",
+    "postinstall": "npm install --prefix gutenberg",

@hypest
Copy link
Contributor

hypest commented Jun 19, 2020

but @hypest was unable to have that work this today

On 6e234e4 that did my testing, I consistently see the Markdown block not working on the Pixel 2XL Android 10 but works OK on S7Edge Android 8. So, I think this is probably a webview issue between the OS/vendor versions, not a Monorepo issue.

@hypest
Copy link
Contributor

hypest commented Jun 19, 2020

I can confirm the error: patch failed situation. The error happens unless we delete the node_modules folders.

@marecar3
Copy link
Contributor

marecar3 commented Jun 22, 2020

There are two items I could use feedback on this PR @mchowning @marecar3

  1. wp.BUILD_GUTENBERG_FROM_SOURCE = true, could you check to see if editing an unsupported block works for you? It's been working for me, but @hypest was unable to have that work this today

Hey @cameronvoell, I couldn't reproduce the issue that @hypest has experienced while testing an unsupported block on monorepo branches.

Also, could you maybe update this branch with the latest develop branch as it has some fixes which are related to an unsupported block project? thanks.

@hypest
Copy link
Contributor

hypest commented Jun 23, 2020

Also, could you maybe update this branch with the latest develop branch as it has some fixes which are related to an unsupported block project? thanks.

What are the fixes @marecar3 ? The plan is to not update the monorepo branches unless really needed. The various projects can then target the post-monorepo branches with any fixes or for releasing features.

@marecar3
Copy link
Contributor

Also, could you maybe update this branch with the latest develop branch as it has some fixes which are related to an unsupported block project? thanks.

What are the fixes @marecar3 ? The plan is to not update the monorepo branches unless really needed. The various projects can then target the post-monorepo branches with any fixes or for releasing features.

Hey @hypest 👋
This is the fix: #12222
I wasn't aware that we can't update WPAndroid PR with the latest develop.

@hypest
Copy link
Contributor

hypest commented Jun 23, 2020

Ah, looks like 12222 has already been merged to WPAndroid develop so, the monorepo PR would need to update to that or we might need to revert 12222 and put it on the "after-1.31" branch. cc @cameronvoell , @mchowning

@mchowning mchowning changed the base branch from develop to gutenberg/integrate_release_1.31.0 June 23, 2020 20:14
@mchowning
Copy link
Contributor

mchowning commented Jun 23, 2020

Updated this PR to target the gutenberg release branch and did a very quick smoke test of this with the submodule update in e0c2c8e, and the editor is loading well for me with both from-source builds and builds that generate the bundle. 👍 I think we can go ahead and merge this to the release branch.

Ah, looks like 12222 has already been merged to WPAndroid develop so, the monorepo PR would need to update to that or we might need to revert 12222 and put it on the "after-1.31" branch.

I'm only seeing a pretty very small fix in WPAndroid with #12222 . If that's it, I'm inclined to favor just keeping it in the release. And we can change our mind later, and still revert it if we want.

@mchowning mchowning merged commit 576ab90 into gutenberg/integrate_release_1.31.0 Jun 23, 2020
@mchowning mchowning deleted the gutenberg/enable-build-from-monorepo branch June 23, 2020 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants