[HOLD for payment 2022-12-23] [$1000] Web splash screen does not work correctly on dev

[roryabraham]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

Run the dev web app.

Expected Result:

The JS code for splash screen (and the .svg component) should be inlined in index.html instead of included in a separate bundle.

Actual Result:

This works as expected on staging and production, but not on dev. On dev the splash screen JS ends up in a separate bundle, not inlined.

Workaround:

n/a – only affects developers.

Platform:

Where is this issue occurring?

• Web
• Desktop App
• Mobile Web

Version Number: 1.2.32-1
Reproducible in staging?: no
Reproducible in production?: no
Notes/Photos/Videos: I created an issue over here to ask the library maintainer about this.

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ea03aab22a5c88d1
• Upwork Job ID: 1597497576249335808

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e02c14f4b93626af
• Upwork Job ID: 1603853587688243200
• Last Price Increase: 2022-12-16

[melvin-bot]

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[roryabraham]

Confirmed that removing SpeedMeasurePlugin altogether fixes the issue. Going to see if we can employ this workaround

[roryabraham]

Unfortunately none of the workarounds listed in stephencookdev/speed-measure-webpack-plugin#167 seem to work to resolve the issue.

[roryabraham]

Going to make this external and see if we can get a fix that doesn't force us to give up speed-measure-webpack-plugin completely, since it is a useful tool for what it does.

[melvin-bot]

Current assignee @CortneyOfstad is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01ea03aab22a5c88d1

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (External)

[melvin-bot]

Current assignee @roryabraham is eligible for the External assigner, not assigning anyone new.

[JalalMitali]

I can't reproduce this issue on Safari MacOS

[mdneyazahmad]

@JalalMitali make this changes in order to reproduce. Please ignore this, I did not notice the word Safari. You are right I can also not reproduce the issue on safari, but this is reproducible on chrome.

diff --git a/config/webpack/webpack.dev.js b/config/webpack/webpack.dev.js
index 2a418f882..f692d2147 100644
--- a/config/webpack/webpack.dev.js
+++ b/config/webpack/webpack.dev.js
@@ -59,5 +59,5 @@ module.exports = (env = {}) => portfinder.getPortPromise({port: BASE_PORT})
             },
         });
 
-        return speedMeasure.wrap(config);
+        return config;
     });

[WestDon]

Proposal

Update webpack.dev.js
with following lines:

        const confPlugins = config.plugins;
        
        const pluginsToSkipMeasure = confPlugins.filter(item => item.constructor.name === 'HtmlWebpackPlugin' || item.constructor.name === 'HtmlInlineScriptPlugin');
        
        config.plugins = confPlugins.filter(x => !pluginsToSkipMeasure.includes(x));

        const configWithTimeMeasures = new SpeedMeasurePlugin().wrap(config);

        configWithTimeMeasures.plugins.push(...pluginsToSkipMeasure);

        return configWithTimeMeasures;

The following plugins, HtmlWebpackPlugin and HtmlInlineScriptPlugin, don't work with HtmlWebpackPlugin as excepted, so the idea is for the dev to exclude them from SpeedMeasurePlugin wrapper

[roryabraham]

Not sure how viable it is, but I would love to see a proposal for an upstream fix in SpeedMeasurePlugin that doesn't rely on a workaround like this.

[ShuiRuTian]

I believe #13182 will fix the issue, and it will also improve("fix" might be more proper) the build/rebuild speed.

The fix PR replace SpeedMeasurePlugin with time-analytics-webpack-plugin, because SpeedMeasurePlugin is almost dead and time-analytics-webpack-plugin fixes some situations(like this).

And, as the author of time-analytics-webpack-plugin, any feedback is welcomed.

[Santhosh-Sellavel]

@ShuiRuTian

Please read our contributing guidelines to understand our process.

Also Refer proposing a solution to the job.

So as first step close the PR and write a proper proposal here thanks!

[ShuiRuTian]

Proposal

What we need to do:

Replace speed-measure-plugin with time-analytics-webpack-plugin.

Reason:

1. speed-measure-plugin will make custom hooks of other plugins go wrong. This will cause some strange issues. I guess this issue is just caused by this reason.
2. This replacement will also fix the build speed, In my machine, the first build time is reduced from 47s to 34s, and the rebuild time is reduced from 10s to 3s. Not sure whether this is due to the correct use of the plugin or webpack could reuse some internal cache.
3. I am the author of time-analytics-webpack-plugin. I want feedback from real users. And if there is any regression, I could provide response as soon as possible. It's win-win.

Tech details:

"tap" methods are the callback that every webpack plugin will use to do something at some time point.

speed-measure-plugin uses Proxy to take over the control of "tap" methods, so that it could measure the time.

However, the reference of Proxy object and the origin target is different. So Map and WeakMap will treat them as different key, then the value is not correct either.

time-analytics-webpack-plugin hack for such keys, so the plugin will work.

[Santhosh-Sellavel]

Thanks, @ShuiRuTian, seems we are not likely to give up the plugin based on this comment.

@roryabraham Again to confirm can we consider this proposal?

[roryabraham]

@ShuiRuTian I'm intrigued.

@Santhosh-Sellavel I'm willing to consider @ShuiRuTian's proposal since I agree it seems like https://github.com/stephencookdev/speed-measure-webpack-plugin is close to EOL:

• Is this repo still under maintenance? stephencookdev/speed-measure-webpack-plugin#187
• Incompatibility with some other webpack plugins stephencookdev/speed-measure-webpack-plugin#160
• Does it work with webpack 5? stephencookdev/speed-measure-webpack-plugin#149
• When can webpack 5 be perfectly supported? stephencookdev/speed-measure-webpack-plugin#174

[roryabraham]

Looks like the PR was merged

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.40-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• use time-analytics-webpack-plugin #13399

If no regressions arise, payment will be issued on 2022-12-23. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@Santhosh-Sellavel / @roryabraham] The PR that introduced the bug has been identified. Link to the PR:
• [@Santhosh-Sellavel / @roryabraham] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@Santhosh-Sellavel / @roryabraham] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@CortneyOfstad] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

[CortneyOfstad]

Heading OoO for the next two weeks (which is over the held payment date) so reassigning

[melvin-bot]

Triggered auto assignment to @davidcardoza (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01e02c14f4b93626af

[melvin-bot]

Current assignee @Santhosh-Sellavel is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Current assignee @roryabraham is eligible for the External assigner, not assigning anyone new.

[Santhosh-Sellavel]

I guess this PR qualifies for a 50% bonus, as the delay was due to some other lint issue.

[mallenexpensify]

@ShuiRuTian , @Santhosh-Sellavel can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~01ea03aab22a5c88d1

[ShuiRuTian]

@mallenexpensify @Santhosh-Sellavel
Thanks. I accepted the job just now on upwork.

[mallenexpensify]

Paid @ShuiRuTian and @Santhosh-Sellavel, $1000 each. The bonus timeline doesn't reflect this issue - Dec 6th hired, Dec 14th merged.

@Santhosh-Sellavel can you check off these please?
#13123 (comment)

@CortneyOfstad , please do too, I'll try to help over break if I have time

[Santhosh-Sellavel]

@mallenexpensify
Regarding the checklist

There is no offending PR here as it doesn't cause any issues on production. Also, this is a development-only case where we just migrating to a new library for measuring build stats, so please anyone check all off (I don't have permission)

cc: @roryabraham

[mallenexpensify]

Thanks @Santhosh-Sellavel, I'm still new to the BZ checklists.
So.. even though the issue is unlocked, you still can't check the boxes, I didn't know that. I addressed in a #contributor-plus thread here.

I missed the 'on dev' part (yes, even though it's clearly in the title). Based on that, I think we can close this for now. comment/reopen if needed