Add support for lazy-loading by requiring octokit/lazy, rather than just octokit
#1431
Conversation
|
@gmcgibbon How do you feel about this approach? It allows you to opt-in for faster boot times, whilst not ending up being a breaking change in certain cases. |
| octokit_require = ENV.fetch('OCTOKIT_REQUIRE', 'octokit') | ||
| require octokit_require | ||
|
|
||
| require 'faraday' |
There was a problem hiding this comment.
We have to manually require Faraday because, if we end up requiring the "lazy" version of Octokit above, then Faraday won't have been loaded when we try to look at its version later in this file.
There was a problem hiding this comment.
I think that you can still ship an autoloaded version of the gem without having to opt-in, it will just require a bigger refactor of each file. If we make it so each file only defines one module (and no peer modules like the error.rb file does by defining several error classes, there should be no problem).
The issue here is that that could take some time, and this approach will work for now, but the lazy loaded version of the client in this PR has the same bugs that will eventually need to be fixed.
Since I'm not the maintainer of this gem, it is not my decision, but I would refactor the file structure and then try re-introducing autoloading when this is done.
|
Thanks for taking a look at this so quickly ❤️ You're right - it would be possible to use However, even with that in mind, I'd still argue that it might be better for lazy-loading to be optional. Some users would likely prefer the "work" to happen when their app boots, rather than adding "warmup time" to their first request (as an example). What do you think? If we do want to provide optional lazy loading, then we could merge this version and then improve the lazy loading process as a follow-up. |
You could also just use Zeitwerk and support optional eager loading and autoloading like Rails does to be more flexible. However, I think it would be useful to look at benchmarks or profiles of the load time added by autoloading to a first request before deciding on anything. As an intermediary step, we can prep the repo for autoload support by organizing the modules and files like I mentioned because Zeitwerk also requires this (as does Rails). |
|
I did consider Zeitwerk, but I was a little wary of adding runtime dependencies and I wasn't sure how it worked in the gem context. I'm happy to pause this PR, do some more digging and come back. I do think this solution is a good start, but it might be worth skipping straight to something better. Thank you for your feedback and for the original PR - I hope we can bring these performance improvements out soon. |
|
👋 Hey Friends, this pull request has been automatically marked as |
github-actions
bot
added
Status: Stale
|
👋 Hey Friends, this pull request has been automatically marked as |
#1351 introduced support for "lazy loading" in the library rather than loading everything upfront. @gmcgibbon ran some benchmarks and found that this approach shaved 400ms off their application startup.
This was a nice idea - however, it ended up unexpected being a breaking change because users were relying on all classes and modules in Octokit being loaded upfront (e.g. so
rescue Octokit::TooManyRequestsworks). We ended up reverting the change in #1428.This introduces new, opt-in lazy-loading. All you need to do to take advantage of it is
require 'octokit/lazy'instead of requiring plan old'octokit'.In our CI run, we test the gem with both methods of requiring, which should help to avoid any unexpected surprises when we release changes.