Remove auto-generated files from bundle and add to .gitignore

[cameronvoell]

Fixes #2693

This change removes autogenerated files in the bundle directory from source control.

WPAndroid Test PR:wordpress-mobile/WordPress-Android#13068
WPiOS Test PR: wordpress-mobile/WordPress-iOS#15060

To test:
WPAndroid

1. Run a local WPAndroid build and make sure everything works as expected
2. Test Automated WPAndroid build and make sure everything works as expected

WPiOS

1. Run a local WPiOS build and make sure everything works as expected
2. Test Automated WPiOS build and make sure everything works as expected

DemoApp
3. Run Demo app from gutenberg-mobile on Android and iOS and make sure everything works as expected
.4 Run demo app from gutenberg on Android and iOS and make sure everything works as expected.

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]

👋 Cameron, I had a quick look at the diff and here are my thoughts:

• bundle/android/raw/ AFAIK, those are not needed in the Android apps so, I'm cool with removing+ignoring
• bundle/android/App.js and bundle/android/App.js.map. I think I'm on the fence for these ones. We could have build improvements that would rely on this file for rebuilding or not from source so, it makes sense to keep it in VCS. Also, we traditionally kept them there so to "match" the iOS bundle which is the one really needed to passed WPAndroid. But, what do you think @cameronvoell? Can you elaborate on why removing those?
• bundle/ios/assets/* I think @etoledom has commented in the past that we don't need those. But @Tug, I think you have commented the opposite on Slack so, can you elaborate on whether we need those .json files or not?
• I see some diff on a translations file: https://github.com/wordpress-mobile/gutenberg-mobile/pull/2694/files#diff-fadcda321726c553aff8098335f23ab6. Is that indeed part of the changes here? It doesn't look like removing "auto-generated" file and I'd prefer to not have them in this PR unless needed. Can you elaborate @cameronvoell? Thanks!

[etoledom]

About bundle/ios/assets/*, I remember had commented about the block.json files, since I thought have seen them in the WPiOS App bundle at some point, but then searching for them I didn't see them, so I can't be sure if they are really needed or not.

assets/ also holds the localisation strings, those seem to be important 🤔

I haven't been involved in the addition of any of those files so I can't really be completely sure if they are needed or not without further investigation.

[Tug]

But @Tug, I think you have commented the opposite on Slack so, can you elaborate on whether we need those .json files or not?

It doesn't seem like those are needed anywhere from what I can see. I can find all json files inlined inside the bundle itself. I think we should definitely stop versioning those 👍

[cameronvoell]

bundle/android/App.js and bundle/android/App.js.map. We could have build improvements that would rely on this file for rebuilding or not from source so, it makes sense to keep it in VCS. Also, we traditionally kept them there so to "match" the iOS bundle which is the one really needed to passed WPAndroid. But, what do you think @cameronvoell? Can you elaborate on why removing those?

@hypest My thinking was that the primary issue I was seeking to address is gutenberg-mobile files showing as changed after building from WPAndroid (#2693), and the quickest way to achieve that is to remove the files from VCS that are currently being generated on first (wp.BUILD_GUTENBERG_FROM_SOURCE = false) WPAndroid build anyway, App.js/App.js.map included. I agree it makes sense to have the App.js/App.js.map files under VCS if they are being utilized for build improvements, but I'll suggest that we re-add the files to VCS at the time build-improvements are added.

Alternatively, we could just do the smaller clean up job of removing bundle/android/raw in this PR and hold off on the short term improvement of no changing files in gutenberg-mobile dir on build for WPAndroid devs, and instead focus on updating the bundle output to be deterministically generated from gutenberg-mobile alone (not include local filepaths), or wait until the WPAndroid build does not re-write the bundle on initial builds (I think there may be non obvious reasons why we're doing this currently and would need to dig in more).

[cameronvoell]

assets/ also holds the localisation strings, those seem to be important 🤔

@etoledom Re-added translation files 62a14e8 (should see they're no longer in the PR file diff)

I see some diff on a translations file: https://github.com/wordpress-mobile/gutenberg-mobile/pull/2694/files#diff-fadcda321726c553aff8098335f23ab6. Is that indeed part of the changes here? It doesn't look like removing "auto-generated" file and I'd prefer to not have them in this PR unless needed. Can you elaborate @cameronvoell? Thanks!

@hypest That change was just from running npm run bundle and committing changes for an accurate WPiOS test. Changes are removed now.

[hypest]

I agree it makes sense to have the App.js/App.js.map files under VCS if they are being utilized for build improvements, but I'll suggest that we re-add the files to VCS at the time build-improvements are added.

Thanks for elaborating Cameron! I should have been more clear: I think that currently we do rely on the bundle file existence in the build pipeline, to determine if the bundle needs to be rebuilt or not. See https://github.com/WordPress/gutenberg/blob/736890da82c4eeb479ee9f1b5b12fadd3bf1dbb6/packages/react-native-bridge/android/build.gradle#L202. So, my assumption is that (again, currently) when we commit the bundle we bring it up-to-date with the sources and Gradle is able to avoid rebuilding it. That said, I'm not exactly sure that this optimization does indeed happen in practice.

Until we figure this out, yeah, I'd opt for not removing the bundles yet but totally think we need to remove the block.json files which are way more numerous too, adding too much noise. WDYT?

[mchowning]

I think that currently we do rely on the bundle file existence in the build pipeline, to determine if the bundle needs to be rebuilt or not. See https://github.com/WordPress/gutenberg/blob/736890da82c4eeb479ee9f1b5b12fadd3bf1dbb6/packages/react-native-bridge/android/build.gradle#L202. So, my assumption is that (again, currently) when we commit the bundle we bring it up-to-date with the sources and Gradle is able to avoid rebuilding it. That said, I'm not exactly sure that this optimization does indeed happen in practice.

We do not check that bundle file to see if it is up-to-date currently in the build pipeline. Instead, we only check if the bundle file that we generate and copy to a build directory in the react-native-bridge project is up-to-date. Line #202 referenced above just ignores the bundle directory when looking for changes (note the ! in the logic). That is needed to avoid rebuilding when there are changes in the bundle directory (which happens just about every time we generate a bundle in the build pipeline). Basically, from a build perspective, I think we can remove the Android bundle files from the bundle directory (we should, of course, confirm that by running a build with them removed, which I haven't done).

My thinking was that the primary issue I was seeking to address is gutenberg-mobile files showing as changed after building from WPAndroid (#2693), and the quickest way to achieve that is to remove the files from VCS that are currently being generated on first (wp.BUILD_GUTENBERG_FROM_SOURCE = false) WPAndroid build anyway, App.js/App.js.map included.

That build pipeline is going to be changed to no longer regenerate the bundle files in the next few months (@oguzkocer already has a rough working prototype, and I believe the work is expected for November), so I don't think we need to worry too much about optimizing for that process.

bundle/android/App.js and bundle/android/App.js.map. I think I'm on the fence for these ones. We could have build improvements that would rely on this file for rebuilding or not from source so, it makes sense to keep it in VCS. Also, we traditionally kept them there so to "match" the iOS bundle which is the one really needed to passed WPAndroid.

Those reasons do resonate with me. At the same time, they are a bit speculative (does being inconsistent with iOS on this really hurt us, and we don't know whether future build improvements will need them.) That makes me think it may be worthwhile to go ahead and remove those files to see how things go. It will be easy to add them back later if/when we discover we need them. In the end, I don't feel strongly either way.

[hypest]

does being inconsistent with iOS on this really hurt us(?)

That's exactly how I feel too at this stage @mchowning. We wanted to have the bundles be up-to-date corresponding to the source code, but after more than two years in the project we never really depended on that mirroring of the bundle files themselves. And, if as you said in your comment (wow, ! are hard to notice these days :whistle:) we don't even use it for build optimization, I'd say let's go ahead and nix it for Android!

[cameronvoell]

To summarize current state:

• I addressed the concern @etoledom raised about translation json files here: Remove auto-generated files from bundle and add to .gitignore #2694 (comment) by re-adding the translations.json files in this change 62a14e8
• I addressed android diff on translations, that @hypest mentioned at the end of his comment here: Remove auto-generated files from bundle and add to .gitignore #2694 (comment)
• We verified with @mchowning and @hypest that Android bundle/android/App.js and bundle/android/App.js.map are not currently utlized by the build optimizations.

I also performed the checks from the PR description, and CI passed on this PR, as well as on the test PRs for WPAndroid, and WPiOS.

As an additional protection, I'll be wrangling the next release and will be able to restore these files if we see any issues with the release.

Let me know if there are any other concerns needed for PR approval @hypest @mchowning @etoledom 🙏

[hypest]

LGTM!