Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use autoload instead of require/require_relative #445

Merged
merged 3 commits into from
Jul 25, 2024
Merged

Use autoload instead of require/require_relative #445

merged 3 commits into from
Jul 25, 2024

Conversation

jhollinger
Copy link
Contributor

@jhollinger jhollinger commented Jul 18, 2024

Checklist:

  • I have updated the necessary documentation
  • I have signed off all my commits as required by DCO
  • My build is green

Per #438, start using autoload for some things, and use require 'blueprint/... instead of require_relative in other places. This allows parts of the codebase to be loaded only if they're needed (Extensions, Transformers, V2, etc).

Based on feedback, the logic of autoload vs require is now:

  • Use autoload for optional, top-level constants (Base, a future V2, Extension, Transformer, etc).
  • Autoloaded files should require any constants they directly reference.
  • Errors are an exception - we'll probably have an increasing number of them, so autoload makes IMHO.
  • The above rules should ensure that nothing is autoloaded during render, except for errors.

@jhollinger jhollinger force-pushed the autoload branch 2 times, most recently from 6a06e67 to b4a2051 Compare July 18, 2024 21:00
@jhollinger jhollinger marked this pull request as ready for review July 18, 2024 21:02
@jhollinger jhollinger requested review from ritikesh and a team as code owners July 18, 2024 21:02
@@ -0,0 +1,7 @@
# frozen_string_literal: true
Copy link
Contributor

@friscomm friscomm Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This autoload functionality is new to me, so this is likely a basic question - why is this autoload separate from the others in lib/blueprinter.rb?

Is it more of an organization thing? Like to avoid putting all of the required autoloads in one massive file?

Or is there some valuable functional difference by putting it here? Do we gain anything performance-wise by separating this out?

Just trying to further my understanding as I'm learning about it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, purely organizational.

Copy link
Contributor

@lessthanjacob lessthanjacob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be curious to hear your thoughts on whether we should broadly use autoload everywhere, or if we should only focus on code/modules that will end up being optional, or truly "modular" in the future.

I definitely agree that being able to selectively (lazily) load functionality in future updates will be a fantastic add, but I imagine we'll likely find ourselves in a place where there's still consistent, essential dependencies (e.g. Blueprinter::Base will always leverage View). In those scenarios, I'd argue that require captures the intent more, and should be preferred over autoload.

Additionally, while our performance/efficiency margins are small within this gem, it's worth noting that we're optimizing for a smaller memory footprint and reduced start-up time at the expense of the first render. I'd argue that start-up time is less important than time to first render in the context of a production application, but I think there's a balance to be made here.

Essentially, my TL;DR here is that I think we should still use require for core dependencies, and focus on leveraging autoload for optional/modular functionality (the pattern for this might not be apparent yet, but we could always revisit in the future).

All that said, I'm open for to this approach if we're comfortable with the tradeoffs, and it aligns with our longer term vision.

def self.configuration
@configuration ||= Configuration.new
end
module Configurable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding another module, how do we feel about pulling forward this change and providing the configuration hooks in Blueprinter directly?

Same general intent, but I don't think we need to encapsulate this in another module. That said, if we feel strongly about this, I'm supportive!

Rakefile Outdated Show resolved Hide resolved
@jhollinger
Copy link
Contributor Author

I'd be curious to hear your thoughts on whether we should broadly use autoload everywhere, or if we should only focus on code/modules that will end up being optional, or truly "modular" in the future....

@lessthanjacob I went "whole hog" b/c I didn't love the idea of loading things two different ways. You make good points, though. How about:

  • Base
  • V2 (or however that shakes out)
  • Extension?
  • Errors:*?

jhollinger and others added 2 commits July 22, 2024 18:28
Co-authored-by: Jake Sheehy <[email protected]>
Signed-off-by: Jordan Hollinger <[email protected]>
Signed-off-by: Jordan Hollinger <[email protected]>
Signed-off-by: Jordan Hollinger <[email protected]>
@jhollinger jhollinger marked this pull request as ready for review July 23, 2024 19:25
@jhollinger
Copy link
Contributor Author

Updated with a hybrid autoload/require approach (see updated description).

@jhollinger jhollinger merged commit f99f5f6 into main Jul 25, 2024
7 checks passed
@jhollinger jhollinger deleted the autoload branch July 25, 2024 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants