Bundle local static React Native assets for Android

[dcalhoun]

• WPiOS: Bundle local static React Native assets WordPress-iOS#16967

Now that we are attempting to load local static image assets in the React Native bundle in WordPress/gutenberg#33790, we must copy those assets to the react-native-bridge app for them to properly show up in the WordPress Android app.

To test:

1. Use an app build that does not rely upon a local React Native Metro server.
2. Open the block editor.
3. Tap the three-dot menu button in the top-right corner.
4. Tap Help.
5. Tap one of the available articles.
6. ℹ️ Expected: Verify the images are displayed and not missing.

Screenshot of Help Image

PR submission checklist:

• I have considered adding unit tests where possible.
• I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

[peril-wordpress-mobile]

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

[hypest]

👋 @dcalhoun ! Tried this PR out from a WPAndroid checkout locally and it works nicely! Good job!

Couple of things I noticed:

1. Not all images included by the Gutenberg PR are appearing in the final Android library artifact (.aar file). Is that expected? I guess they are not getting processed by Metro since no code is referencing them? For example, what-is-a-block-png is not processed. Can you check? Thanks!
2. Along with the editor onboarding image assets, it looks like the copy script is pulling in a few more assets, namely:

gutenberg_node_modules_reactnavigation_stack_src_views_assets_backicon.png
gutenberg_node_modules_reactnavigation_stack_src_views_assets_backiconmask.png
jetpack_projects_plugins_jetpack_extensions_blocks_story_story_example1.png

Is that intentional? Can you check whether this can cause some issue? Thanks!

[dcalhoun]

@hypest thank you for the thorough review.

1. Not all images included by the Gutenberg PR are appearing in the final Android library artifact (.aar file). Is that expected? I guess they are not getting processed by Metro since no code is referencing them? For example, what-is-a-block-png is not processed. Can you check? Thanks!

It would appear what-is-a-block.png is an erroneous, unused file. I believe you are correct that because the JavaScript contains no references to the asset, Metro is intentionally not placing the asset within the "drawable" directories while bundling. I will remove the unused image in a separate PR when I work to finish the Help Section detail view in Gutenberg.

2. Along with the editor onboarding image assets, it looks like the copy script is pulling in a few more assets, namely:

gutenberg_node_modules_reactnavigation_stack_src_views_assets_backicon.png
gutenberg_node_modules_reactnavigation_stack_src_views_assets_backiconmask.png
jetpack_projects_plugins_jetpack_extensions_blocks_story_story_example1.png

Is that intentional? Can you check whether this can cause some issue? Thanks!

It is intentional in the sense that Metro copies all images that are referenced by the JavaScript bundle. I do not believe it causes issue.

I believe the first two images are referenced/provided by the third-party React Navigation. We do not leverage them in our first-party code currently. I do not believe Metro does any "tree shaking," so the third-party reference likely causes the image to be bundled.

The last image was added as the Story block preview image used in the web editor while hovering over a block within the inserter. Because the native client uses the same JavaScript file, Metro likely includes the asset in the bundle.

We could potentially make the script in this PR more robust, replacing a simple cp with a rsync or something similar to exclude unneeded files. However, excluding these images would be somewhat hidden and may surprise someone if they attempt to reference an image we exclude here. I am not sure it is worth excluding these few, small images. WDYT?

[hypest]

because the JavaScript contains no references to the asset, Metro is intentionally not placing the asset within the "drawable" directories while bundling

Thanks for confirming! I'd suggest augmenting the inline comment to mention that all local images referenced in the JS code will be processed and copied to the app binary, as a quick headups to people. wdyt?

However, excluding these images would be somewhat hidden and may surprise someone if they attempt to reference an image we exclude here. I am not sure it is worth excluding these few, small images. WDYT?

Agreed, I think it would surprising to find that not all images end up in the app binary. Also, if we go ahead and augment the inline comments we can make that clear.

[dcalhoun]

@hypest good idea. Addressed in fde69a8.

[hypest]

From the nitpicking department: I'm slightly confused by the format of the references at the top of the PR description David 🤔. For one, the description starts with a bullet, making me expect more than one bullets and since it's customary to have the links to both the WPiOS and the WPAndroid PRs, feel like the PR is missing the link to the WPAndroid PR.
Then, when looking at the WPiOS PR, I see the similar leading bullet trying to reference a WPAndroid link, but it's actually providing the gutenberg-mobile PR link.

Maybe I'm too accustomed to past PRs but, can you elaborate on the format? Like, what do you have in mind when sharing the sister PRs first (in a bullet at that) and then actually having the description of what this PR is about? Again, nitpicking but curious at this stage since I see it in a few of your PRs 🤔, thanks!

[hypest]

LGTM!

I will remove the unused image in a separate PR when I work to finish the Help Section detail view in Gutenberg.

👍, thanks!

[dcalhoun]

For one, the description starts with a bullet, making me expect more than one bullets and since it's customary to have the links to both the WPiOS and the WPAndroid PRs, feel like the PR is missing the link to the WPAndroid PR.

@hypest using a list is carry over from the practice of referencing the multiple PRs that are generally required for Gutenberg Mobile work. This particular work happens to only have two total PRs, hence the single bullet. Would avoiding lists when there is a singular item improve communication from your perspective?

Then, when looking at the WPiOS PR, I see the similar leading bullet trying to reference a WPAndroid link, but it's actually providing the gutenberg-mobile PR link.

Labeling the linked PR as "WPAndroid" is a mistake. The linked PR, in fact, resides in gutenberg-mobile. The work just happens to only relate to integration of WordPress-Android. I will update the description to be more accurate.

Like, what do you have in mind when sharing the sister PRs first (in a bullet at that) and then actually having the description of what this PR is about?

I observed and was told during onboarding that including links to sibling work was helpful/expected. The format and placement of those sibling links seems to vary throughout the team. Placing the sibling PRs as the first item in the description was merely an attempt for consistency in the PRs across each repository that has its own template.

Maybe I'm too accustomed to past PRs [...]

Humorously, from my perspective, I have largely formatted my PRs based upon with practices I observed from other PRs. 😄 I suppose I may have missed a nuance or taken too much liberty along the way.

Happy to adapt my format if you have a particular preference. Just let me know. 🙇🏻

[dcalhoun]

If you reference an issue, pull request, or discussion in a list, the reference will unfurl to show the title and state instead. For more information about task lists, see "About task lists."

One additional note about lists on GitHub — placing GitHub links in a list causes additional details to unfurl due to the Task List feature. E.g. it displays the title and state instead.

[hypest]

Would avoiding lists when there is a singular item improve communication from your perspective?

Ah, I think so. Thanks!

Placing the sibling PRs as the first item in the description was merely an attempt for consistency in the PRs across each repository that has its own template.

Oh, I see. I guess keeping the important bit first (what's this PR about) is easier to my eyes as a reader, having the links to relevant PRs as part of the context that follows the main description. My understanding is that that's already the typical format around the repos?

And yes, thanks for mentioning GitHub's unfurling... I think it does seem to make things slightly worse by adding more words, ending up kinda noisy when the links precede the actual description.

[dcalhoun]

Oh, I see. I guess keeping the important bit first (what's this PR about) is easier to my eyes as a reader, having the links to relevant PRs as part of the context that follows the main description. My understanding is that that's already the typical format around the repos?

My perception is the "typical" is not that consistent, and most PR descriptions I see begin with references to sibling PRs. So, I am not too sure of the preference of the team at large.

And yes, thanks for mentioning GitHub's unfurling... I think it does seem to make things slightly worse by adding more words, ending up kinda noisy when the links precede the actual description.

Personally, I prefer the unfurling. However, I'll avoid them moving forward.

[hypest]

Personally, I prefer the unfurling. However, I'll avoid them moving forward.

oh wait, I think I better see what you mean now. That no matter how many the bullets, you still prefer it as it provides the unfurling, right? Please ignore me on that one, I don't see the need for revising since it does provide more context to you 👍

About the what comes first (the bullets or the description of the purpose of the PR) I still consider it a nitpick from my side and now that I have more insight on your goals I'm more than fine with the initiative ✅ so, feel free to develop it further. It might even be helpful to share within the team as an attempt to add consistency where's due. Thanks!