i18n-calypso: Replace EventEmitter with simplified subscriber callbacks #103304
Conversation
matticbot
added
the
[Status] Needs Review
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~195 bytes removed 📉 [gzipped]) Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~241 bytes removed 📉 [gzipped]) Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~51 bytes removed 📉 [gzipped]) React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
There was a problem hiding this comment.
The changes appear to be working well, and the rationale behind them makes sense. Regarding the suggestion to use a Set instead of an array for the subscribers - I haven't done a deep dive into the performance implications, so feel free to disregard that if using an array makes more sense in this context.
LGTM 👍
| @@ -328,6 +328,6 @@ export function trackTranslatorStatus( isTranslatorEnabled ) { | |||
| } | |||
|
|
|||
| // re-initialize when new locale data is loaded | |||
| i18n.on( 'change', communityTranslatorJumpstart.init.bind( communityTranslatorJumpstart ) ); | |||
| i18n.subscribe( communityTranslatorJumpstart.init.bind( communityTranslatorJumpstart ) ); | |||
There was a problem hiding this comment.
The call of the init method, does it even work? The method takes two args, user and isUserSettingsReady. And returns early if user is null-ish. But the change event listener was not called with any arguments, is it? And neither is the new subscribe callback.
There was a problem hiding this comment.
The call of the
initmethod, does it even work? The method takes two args,userandisUserSettingsReady. And returns early ifuseris null-ish. But thechangeevent listener was not called with any arguments, is it? And neither is the newsubscribecallback.
I'll check on this tomorrow 👍
There was a problem hiding this comment.
The call of the
initmethod, does it even work? The method takes two args,userandisUserSettingsReady. And returns early ifuseris null-ish. But thechangeevent listener was not called with any arguments, is it? And neither is the newsubscribecallback.I'll check on this tomorrow 👍
Update: You're correct that the initialization triggered by this subscriber always short-circuits at the user check. This subscriber is very old (originates in pre-OSS version), at which time the init function didn't take arguments. Additionally, I don't think we should rely on a side-effect like this when importing a module, since it isn't compatible with tree-shaking. That being said, the subscriber is being added.
Instead, I think the initialization is happening in the TranslatorLauncher component, which passes the user arguments correctly. It also appears to reinitialize many times when the component re-renders.
I'm not entirely familiar with how this should be working (e.g. in response to asynchronous loads or language changes), but I think it's fair to say that the subscriber is not doing anything in its current form and can be removed.
There was a problem hiding this comment.
I'm going to keep this in as-is to keep it functionally equivalent, and will follow-up with a pull request proposing to remove the ineffective subscribe call.
Facilitate deletion by reference See: #103304 (comment)
Since we're interested in subscriber behavior in response to certain actions (e.g. setting locale), not relevant to test here, and duplicative. See: #103304 (comment)
github-actions
bot
removed
the
[Status] Needs Review
Part of ARC-129
Proposed Changes
i18n-calypsopackage, substituting a simplified subscriber implementationWhy are these changes being made?
EventEmitterdata approach toward conventions more conventional and compatible with React (e.g.useEffectsubscribers)eventspackage in common dependency (estimated 2kb gzipped)On performance, I discovered that the
eventslibrary does have a surprisingly-performant subscriber removal behavior. Early iterations here included a "readable"Array#filterimplementation (see a8fb2d4), but considering that many of our localized components will remove event listeners once they're unmounted, it seemed worthwhile to micro-optimize. After several rounds of micro-benchmarking, I found a solution which performs even better (~38%) thanevents's implementation, and ~850x better than a naiveArray#filterimplementation in a scenario with 2000 subscribers.Separately, I had explored an implementation which largely maintained the same API if we'd rather reduce the scope of the changes.
Testing Instructions
Pre-merge Checklist