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.

Sign up for GitHub

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

Jump to bottom

React Native: Try to integrate an example React Native project into repository #11533

Closed
wants to merge 1 commit into from
Closed

React Native: Try to integrate an example React Native project into repository #11533

wants to merge 1 commit into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Nov 6, 2018 •
edited
Loading

Description

Proposal for #11491.

It works with the simulator on iOS. I tested only there so far.

I used react-native init gutenberg command to generate a project to test with to narrow down issues to the integration. To move forward we would have to sync the following folders:

  • gutenberg-mobile/android -> gutenberg/android
  • gutenberg-mobile/ios -> gutenberg/ios
  • gutenberg-mobile/src -> gutenberg/packages/mobile

All other files should be moved into gutenberg/packages/mobile if possible so we could avoid polluting the root folder of the repository.

Testing mobile

  • npm run dev:start
  • npm run dev:ios or npm run dev:android

TODO

  • Babel config needs more love. Web and mobile work but it might do something awkward down the road to the web project.
  • Mobile part uses React as is. It should use @wordpress/element instead (it's related to Babel config.
  • Unit tests fail since we use React 16.6.0 which isn't compatible with enzyme.

@gziolo gziolo added [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible [Status] In Progress Tracking issues with work in progress Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Nov 6, 2018
@gziolo gziolo requested review from Tug, youknowriad and hypest November 6, 2018 09:54
gziolo
gziolo commented Nov 6, 2018
View reviewed changes
babel.config.js
@@ -1,18 +1,7 @@
module.exports = function( api ) {
api.cache( true );
api.cache.never();
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I disabled cache to make it easier to debug :)

gziolo
gziolo commented Nov 6, 2018
View reviewed changes
babel.config.js
@@ -25,5 +14,37 @@ module.exports = function( api ) {
],
},
},
overrides: [
{
include: [
Copy link
Member Author

Choose a reason for hiding this comment

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

It's a hack to move forward 😅

gziolo
gziolo commented Nov 6, 2018
View reviewed changes
babel.config.js
},
{
exclude: [
'./index.js',
Copy link
Member Author

Choose a reason for hiding this comment

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

The issue is with JSX pragma when using both RN and WP presets.

@gziolo gziolo self-assigned this Nov 6, 2018
@Tug
Copy link
Contributor

Tug commented Nov 6, 2018

Cool work here 😎
I wonder if it wouldn't be cleaner to move ios and android to the @wordpress/mobile package? Those don't make much sense in the gutenberg repo without it

gziolo
gziolo commented Nov 6, 2018
View reviewed changes
packages/mobile/package.json
"url": "https://github.com/WordPress/gutenberg/issues"
},
"dependencies": {
"react": "^16.6.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

The idea with monorepo is that you put all dependencies in this file.

@gziolo gziolo mentioned this pull request Nov 6, 2018
Closed
youknowriad
youknowriad reviewed Nov 6, 2018
View reviewed changes
package.json
@@ -41,6 +41,7 @@
"@wordpress/i18n": "file:packages/i18n",
"@wordpress/is-shallow-equal": "file:packages/is-shallow-equal",
"@wordpress/keycodes": "file:packages/keycodes",
"@wordpress/mobile": "file:packages/mobile",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shouldn't add the mobile package here to avoid impacting the web a lot. I consider this root package.json as the package.json for the web application. It can also be moved to packages/web (but that's separate).

@gziolo
Copy link
Member Author

gziolo commented Nov 6, 2018

I wonder if it wouldn't be cleaner to move ios and android to the @wordpress/mobile package? Those don't make much sense in the gutenberg repo without it

@Tug you probably could keep it in the subfolder, but then you would have to start an app from a folder which contains index.js, ios and android folders. I think it's doable, but you would have to play a bit with it to ensure that you can load node_modules properly. NODE_PATH might be a way to go.

In general, I wanted to achieve one goal, to have only one node_modules folder, so we wouldn't have to install the same dependencies twice, to use only one lock file, etc.
There might be some political challenges as RN libraries might not play well with the proper licensing schema you see in WordPress world. I think I had to comment out check-licenses scritpt.

I think we shouldn't add the mobile package here to avoid impacting the web a lot. I consider this root package.json as the package.json for the web application. It can also be moved to packages/web (but that's separate).

@youknowriad that align with @Tug's proposal. I'm not quite sure what could land in web folder but I'm happy to explore. It would be convenient to have separate folders for mobile and web to keep the distinction. We would also treat packages as a shared code.

@gziolo
Copy link
Member Author

gziolo commented Nov 13, 2018

I'm closing this one as it was only opened to validate the idea and to show how things could be handled for #11491.

@gziolo gziolo closed this Nov 13, 2018
@gziolo gziolo deleted the try/react-native-integrated branch November 13, 2018 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowriad youknowriad youknowriad left review comments

@Tug Tug Awaiting requested review from Tug

@hypest hypest Awaiting requested review from hypest

Assignees

@gziolo gziolo

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] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gziolo @Tug @youknowriad