Refactor/pid registry #899
Conversation
There was a problem hiding this comment.
LGTM, besides some naming suggestions 👍 In my opinion, it would be better to rename the browser package to module and consolidate these "registries" under a type called Registry in its own file registry.go within the module package. Reads fine: module.Registry. Since they're closely related and contain module globals, it makes sense to streamline them into one package. Is there a specific reason for keeping them separate?
| // pidRegistry keeps track of the launched browser process IDs. | ||
| type pidRegistry struct { | ||
| // PidRegistry keeps track of the launched browser process IDs. | ||
| type PidRegistry struct { |
There was a problem hiding this comment.
Do we still need to say PidRegistry as the package name already implies that?
There was a problem hiding this comment.
Yeah, I was thinking about that too. Would registry.Pid be better? I think for now it might be ok, considering we have things like context.BackgroundContext. As you've pointed out another good suggestion, we might need to refactor this again very soon, so i might leave the name as it until that next refactor 🙂
registry/remote.go
Outdated
|
|
||
| // RemoteRegistry contains the details of the remote web browsers. | ||
| // At the moment it's the WS URLs. | ||
| type RemoteRegistry struct { |
There was a problem hiding this comment.
The package registry implies registry. Should we still say RemoteRegistry?
There was a problem hiding this comment.
Look here: #899 (comment)
tests/webvital_test.go
Outdated
| "webvital_first_input_delay_good": false, | ||
| "webvital_cumulative_layout_shift": false, | ||
| "webvital_cumulative_layout_shift_good": false, | ||
| "browser_web_vital_ttfb": false, |
There was a problem hiding this comment.
Are these changes related to this PR?
There was a problem hiding this comment.
No. This branches off refactor/remote-global-check, and initially the PR was setup to merge back into main. I've changed this PR so that it now merges back into refactor/remote-global-check. This should highlight the one single commit now.
This is a good suggestion. I think we may have more "registries" that we need to create such as |
I like this idea also 👍 |
Now that we have a registry package, the pidRegistry can be moved out of the browser package and into the registry package. This will help isolate the details.
ankur22
force-pushed
the
refactor/pid-registry
branch
from
e6dad9e
to
59eb77e
Compare
|
I'm going to merge this in and work on the refactor that Inanc suggested. It shouldn't take long 🤞 |
Description of changes
This moves the
pidRegistryfrom thebrowserpackage into theregistrypackage. This refactor will help isolate the details to within theregistrypackage.Related to: #889 (comment)
Checklist
Tasks