[No QA] Fix regression on web due to #4760

[kidroca]

cc @marcaaron

Details

Making production web builds using npm run build fails with a webpack loader error
The problem is discovered as a regression here: #4760 (comment)

Because webpack statically analyzes all require calls - even those that might never happen we need to add react-native-flipper to includeModules

Fixed Issues

$ N/A

Tests

Same as the tests here: #4760 (comment)

QA Steps

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

[kidroca]

process.env values are string so we need to check explicitly for "true"
That's how other compile check are made as well (e.g. process.env.ANALYZE_BUNDLE === 'true')

[kidroca]

Ideally we want check that can be evaluated at compile time (such as if (canCapturePerformanceMetrics()) ) to exclude dead code from the bundle
It might be possible to configure babel, or at least webpack (not sure if metro supports this), but it would open the door for other regressions

Another approach is with the platform specific code. We can extract lib/Performance/index.js, lib/Performance/index.native.js and lib/Performance/BasePerformance.js
Base performance is pretty much a blank stub. index.js exports this "blank stub" while index.native.js uses react-native-performance and the flipper integration when canCapturePerformanceMetrics() is true

Say something if you want to open another PR with the platform specific changes (or modify the current one)

[MonilBhavsar]

I don't have much context regarding this, But looking to previous work and PR's. I think we should see if we can configure our bundler to get rid of unused dependencies. If we're bounded by that, I'm fine with platform specific method. As I see, we had that before?
Let's see what @marcaaron says.

Anyways, this PR looks good to me.

[marcaaron]

Say something if you want to open another PR with the platform specific changes (or modify the current one)
I think we should see if we can configure our bundler to get rid of unused dependencies.

I think if this PR fixes the web builds then we should start here. I agree it would make sense to not bundle this code, but as far as we know we don't have an issue related to the JS bundle size. So maybe it's not worth prioritizing?

I found some documentation for how webpack handles this -> https://webpack.js.org/guides/code-splitting/#dynamic-imports

My only question is... does the conditional require work correctly on native? If it's only web that is bundling I'd say this matters less.

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[kidroca]

My only question is... does the conditional require work correctly on native? If it's only web that is bundling I'd say this matters less.

RN does not support Dynamic Imports and doesn't have the await import() syntax yet: facebook/metro#52

Both webpack and metro support the optional and inline requires - we already use them conditionally load a lib

• for RN it gets bundled into the installation source code - no big deal
• for web it gets bundled into app.js - might be a problem as it affects initial webpage load time

For the current item though the overhead added is negligible - about 10kb (there are also 30kb added by react-native-performance, but they are even harder to get rid of as we also conditionally use it in Onyx...)

It's possible to configure webpack to split chunks out of app.js even if they aren't dynamic imports - this should be enough to address the issue, it would extract a chunk like react-native-performance-flipper-reporter.hash.js. Even though it's still compiled it would only be loaded when the condition matches. This a bit advanced to configure properly and it's not esencial ATM

The other approach is "dead code elimination" which can happen at compile time. Since we only set ENV vars at compile time - CAPTURE_METRICS. webpack and metro should be able to statically analyze and derive that certain code will never be evaluated to true and run so they can strip it.
It's again a bit complex to configure (compared to writing .native or .web) as it requires babel plugin(s) and changing conditions to if(process.env.CAPTURE_METRICS === 'true') as these can be statically evaluated to true or false and then discarded

• if(canCapturePerformanceMetrics()) cannot be statically evaluated as it has to be called

---

As a summary the easiest thing to do right now is to write Platform specific code, but it doesn't seem to be worth it just for the current case.
It's very like bundle splitting would be enabled at some point down the road which is one more reason to skip doing anything ATM

[marcaaron]

Thanks for the summary!

It's very like bundle splitting would be enabled at some point down the road which is one more reason to skip doing anything ATM

I agree with this. It does feel inevitable that we will want to optimize the JS bundle - but just isn't our focus right now.

[OSBotify]

🚀 Deployed to production by @roryabraham in version: 1.0.88-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

[botify]

This has been deployed to production and is now subject to a 7-day regression period.
If no regressions arise, payment will be issued on 2021-09-08. 🎊