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

[Monorepo] import gutenberg-mobile as a package #18159

Closed
wants to merge 4,802 commits into from

Conversation

dratwas
Copy link
Contributor

@dratwas dratwas commented Oct 29, 2019

Description

Explores #11491.
Superseeds #17456 started by @Tug

Wordpress-iOS PR - wordpress-mobile/WordPress-iOS#12762
Wordpress-Android PR - wordpress-mobile/WordPress-Android#10668

This is a migration of gutenberg-mobile to gutenberg repository.

New packages added:

  • @wordpress/react-native-editor - playground react-native app and bundle entrypoint
  • @wordpress/react-native-bridge - module which starts react-native inside the client app (WP-iOS/Android)
  • @wordpress/react-native-aztec - react-native-aztec module

Usual yarn commands ran from gutenberg-mobile are now accessible from gutenberg using npm run native

For instance:

yarn start => npm run native start

How has this been tested?

TBD

Screenshots

Types of changes

Add new native packages, for native build, unifying the 2 repositories

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've imported git history
  • I've checked that the licencing is OK
  • I've checked that package release is working as expected (lerna)
  • I've checked that installing dependencies works as expected and that changes to package-lock.json are OK
  • I've checked that we are up to date with gutenberg-mobile develop branch
  • I've informed concerned developers of the change of repository
  • I've ported e2e tests iOS
  • I've ported e2e tests Android
  • I've ported pre-commit hooks including eslint
  • I've ported i18n
  • I've checked that build scripts are up to date with WordPress-iOS
  • I've checked that build scripts are up to date with WordPress-Android
  • I've checked that WordPress-iOS works properly after migration
  • I've checked that WordPress-Android works properly after migration
  • I've made sure that the project can be opened in Android Studio
  • I've made sure that the project can be opened in XCode
  • I've made sure that the demo app for mobile runs
  • I've made sure that I have excluded any binary files, jar, phar... from the patch (gutenberg-mobile has gradle-wrapper.jar committed to the repo though and we might want to fix that later?)
  • I've checked that all CI steps are imported
  • i've checked that the react-native-aztec example app works

mchowning and others added 30 commits September 20, 2019 11:53
Merge master (v1.13.0) back to develop
… the JS side to Native when Underline is enabled on the current selection
@dratwas dratwas requested a review from Tug October 30, 2019 15:13
@gziolo gziolo self-requested a review October 30, 2019 15:28
@@ -1,8 +1,8 @@
const config = process.env.MOBILE ? require( './packages/react-native-editor/babel.config' ) : {
Copy link
Member

Choose a reason for hiding this comment

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

We need to double-check how Babel caching works with envs.

@gziolo
Copy link
Member

gziolo commented Oct 30, 2019

There are a few files added in the root folder. Can you list all of those which must live there so we could double-check when doing the review?

@@ -0,0 +1,280 @@
diff --git a/node_modules/@types/react-native/globals.d.ts b/node_modules/@types/react-native/globals.d.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need that file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it is a path for react-native because:

after import gutenberg-mobile we have react-native-dark-mode as a dependency which requires @types/react-native. @types/react-native/globals.d.ts and typescript/lib/lib.dom declare the same global types. We can not remove lib.dom from tsconfig because @types/react-native doesn't include Window and i get error:

node_modules/@types/reach__router/index.d.ts:9:30 - error TS2304: Cannot find name 'Window'.

9 export type WindowLocation = Window['location'] & HLocation;

I tried all solutions from this issue (DefinitelyTyped/DefinitelyTyped#15960) and few more, only removing the @types/react-native/globals.d.ts fix that.

Seems like Orta is trying to fix that here - microsoft/types-publisher#655

@Tug
Copy link
Contributor

Tug commented Nov 11, 2019

I can't review package-lock.json from github as there are too many changes but I noticed you might have been using a different npm version than the one recommended for this project. I can see many changes that we probably don't want such as "dev": true, being removed at many places. Might be ok though, but let's double check

@@ -0,0 +1,26 @@
package = JSON.parse(File.read(File.join(__dir__, 'packages', 'react-native-editor', 'package.json')))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move this file to @wordpress/react-native-bridge, or could we even?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to move it to packages/react-native-bridge but couldn't make it works with Wordpress-iOS. I don't know why but cocoapods can not read package.json with that configuration.

@@ -0,0 +1,23 @@
require 'json'
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, would be awesome if we didn't add new files at the root

@Tug
Copy link
Contributor

Tug commented Nov 11, 2019

This looks like it's on good tracks 👍

However, given the amount of changes, I think it will be pretty hard to merge this to master at once without introducing errors somewhere.

What do you think about merging to another branch which we can iterate on?

The idea is that this PR would be mostly copying the content of gutenberg-mobile in gutenberg, even if it breaks lerna or tests for the web. Then follow-up PRs would be about reviewing the additions and fixes (Podspecs, tests, etc).

So, in order to make the diff clearer I would suggest removing all your commits that are not in gutenberg-mobile and cherry-pick them in a new PR that targets this new branch. Wdyt?

@dratwas
Copy link
Contributor Author

dratwas commented Nov 13, 2019

The idea is that this PR would be mostly copying the content of gutenberg-mobile in gutenberg, even if it breaks lerna or tests for the web. Then follow-up PRs would be about reviewing the additions and fixes (Podspecs, tests, etc).

So, in order to make the diff clearer I would suggest removing all your commits that are not in gutenberg-mobile and cherry-pick them in a new PR that targets this new branch. Wdyt?

I like that idea and i planned to do something like this when I finish all the stuff related to the building process (Jitpack, cocoapods) etc. Right after that, I would update branch to master and then will create few PRs to make reviews possible :)

@dratwas dratwas mentioned this pull request Nov 19, 2019
6 tasks
@Tug
Copy link
Contributor

Tug commented Oct 19, 2020

Closing this as it is now outdated

@Tug Tug closed this Oct 19, 2020
@Tug Tug deleted the gutenberg-mobile-monorepo branch October 19, 2020 10:25
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) [Status] In Progress Tracking issues with work in progress [Type] Build Tooling Issues or PRs related to build tooling [Type] Project Management Meta-issues related to project management of Gutenberg [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.