Heavily defer requires for faster activation time

[winstliu]

Requirements

• Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
• All new code requires tests to ensure against regressions.

Description of the Change

This PR heavily defers as much work as possible until needed in order to improve activation time (see the drawbacks section for why this isn't all sunshine and rainbows). Without this PR, it takes around 190ms to activate; with this PR, it takes around 8ms. Changes include:

• Deferring requires in teletype-package.js until a command is activated
  • In order to prevent the status bar tile from activating them immediately anyway, direct references to AuthenticationProvider and PortalBindingManager have been replaced with lazy getter functions.
  • Caching AuthenticationProvider, PortalBindingManager, and TeletypeClient once they're constructed. Is this a good idea? I'm not sure if there's a reason a new object is being constructed each time.
• Deferring creating the popover component in PortalStatusBarIndicator until the status bar tile is clicked
  • Again, this is so that PortalStatusBarIndicator and PopoverComponent doesn't require the same modules that we're trying to defer. When the tile is first clicked the deferred activation occurs which calls the lazy getter functions, attempts to sign in, creates the popover tooltip, and then displays it.
• Conditionally requiring only the components necessary to render the popover. Most of the time the PortalListComponent will be the only one displayed, so this changes saves us from having to require the other three components as well.
• Commenting out two console.logs in the activate method. I don't know why, but they were slowing activation by ~40ms when I was profiling startup. It doesn't look like there's a negative performance impact when running Atom regularly though.

Alternate Designs

Of course, the easier way would be to 🔥 the status bar tile entirely and rely only on the commands. But I don't see another way of doing this while keeping the status bar tile.

Benefits

Shaves a significant amount of time off of initial package activation, leading to faster Atom startup.

Possible Drawbacks

There is a noticeable delay when first clicking on the tile to bring up the popover. The delay is potentially long enough that someone might think that Teletype isn't working. A potential way to improve this is to show a loading spinner on initial click, but that's not something that I looked into implementing for this PR.

In addition, the status bar tile will no longer display outdated or initialization errors until first click, because the client won't be initialized until then.

Applicable Issues

Closes #201

[Arcanemagus]

Something I've done when deferring dependency loads is to do an "opportunistic load" of the dependencies in a requestIdleCallback. You can see an example of the way I've done that here.

This way if the idle callback fires before the user attempts interaction there is no delay, but if they interact before then the idle callback essentially does nothing when it does eventually fire.

[winstliu]

💚💚💚!!! Only took 18 tries to get tests passing again.

[winstliu]

I do not have a test plan for this. It'd be best for some scenarios to be tested out before merging this.

[jasonrudolph]

@50wliu: Thanks for caring about startup time! 💟

A few thoughts on these changes:

---

Commenting out two console.logs in the activate method. I don't know why, but they were slowing activation by ~40ms when I was profiling startup.

Nice find! ⚡ I'm in favor of deleting those statements entirely. We added those statements before there was a settings UI for the package. Now that you can see the Pusher key and the Teletype API URL in the settings UI, I think we can remove these statements entirely.

---

There is a noticeable delay when first clicking on the tile to bring up the popover. The delay is potentially long enough that someone might think that Teletype isn't working

I'm pretty worried about this. I dislike that we would introduce a delay the first time you click the portal icon, as that feels like a step backwards from the current experience.

Additionally, I share your concern that this delay might cause people to think that Teletype isn't working.

I don't think I'd be comfortable merging this pull request unless those issues were resolved. #234 (comment) sounds like a nice approach.

---

Even though these changes improve startup time, they also increase the complexity of the code. If we expected Teletype to always be a standalone package, it might be worthwhile to accept that additional complexity. But given that we intend to bundle the teletype package with Atom eventually, snapshots should eliminate most of the startup time impact for us without needing to introduce this additional complexity into the teletype code base. For that reason and for the reason above regarding the delay when first clicking the portal icon, I'm hesitant to move forward with these changes.

Food for thought: Is there a subset of these changes that would A) reduce startup time, B) avoid the delay when clicking the portal icon, and C) avoid introducing additional complexity in the code? If so, I'd love to see a separate PR with just those changes. What do you think?

[winstliu]

snapshots should eliminate most of the startup time impact for us

This sounds nice, but I'd still like to see the impact it has before deciding not to move forward with this PR. For example, the github package experienced large improvements after being bundled (from 375ms to 28ms), but settings-view is still activating very slowly at 193ms.

Most of the added complexity is needed because of the nature of the requires. Since the status bar is consumed immediately on activation, the status bar tile needs to be able to lazy-load everything and hence lazy getter functions need to be passed to it rather than the components themselves. I believe that I can remove the extra caching I added, but that's only a small part of the overall complexity. In addition, to solve the problem with the popup delay, that would require even more code complexity with the addition of requestIdleCallback.

So to summarize, I currently do not believe that reducing startup time (short of snapshotting) is possible without also increasing complexity. Almost all of the activation time comes from the cascading requires.

[jasonrudolph]

snapshots should eliminate most of the startup time impact for us

This sounds nice, but I'd still like to see the impact it has before deciding not to move forward with this PR.

@50Wliu: Fair enough. Let's gather some data to help us figure out how to proceed. 😄 Would you mind creating a branch in atom/atom that adds teletype as a bundled package and measuring the impact that it has on startup time once it's bundled?

[winstliu]

Hmm it looks like it'll take some effort to get teletype working with snapshots.

Error: Cannot require module "../node_modules/teletype/index.js".
To use Node's require you need to call `snapshotResult.setGlobals` first!