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

Autoload resource classes #1356

Merged
merged 1 commit into from
Jan 6, 2025
Merged

Autoload resource classes #1356

merged 1 commit into from
Jan 6, 2025

Conversation

gmcgibbon
Copy link
Member

@gmcgibbon gmcgibbon commented Jan 3, 2025

Description

Please, include a summary of what the PR is for:

What is the problem it is solving?

Implements autoloading on Active Resources classes so they don't load early unnecessarily. This improves load times for Rails applications, especially in instances where they are not used.

What is the context of these changes?

I don't want this gem to load Active Resource early, it causes unnecessary speed regressions in my application's boot time.

What is the impact of this PR?

Shaves about 40ms off require time. The remainder is caused by other gems we're requiring like GraphQL, but it implements autoloading in the latest version. The time savings would be much greater in apps due to the amount of load hooks for Active Resource in place.

How has this been tested?

Test this with:

#!/usr/bin/env ruby
# frozen_string_literal: true

require 'bundler/setup'
require "benchmark"
# require "ostruct"

time = 1000 * Benchmark.realtime do
  require 'shopify_api'
end

puts "Took #{time}ms"

Note, you might have to add racc to the gemfile and require ostruct before the gem require if you're running on Ruby 3.3+.

Please, describe the tests that you ran to verify your changes.

I verified eager loading does in fact load the autoloaded constants. I didn't add anything to the test suite because this is a very old gem version that's on its way out, so we probably don't need continued assurance that autoloading works.

Checklist:

  • My commit message follow the pattern described in here
  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have updated the project documentation.
  • I have added a changelog line.

cc @sle-c

@gmcgibbon gmcgibbon requested a review from a team as a code owner January 3, 2025 20:29
Implements autoloading on Active Resources classes so they don't load
early unnecessarily. This improves load times for Rails applications,
especially in instances where they are not used.
@gmcgibbon gmcgibbon force-pushed the active_resource_autoload branch from 56b883e to d4402f5 Compare January 4, 2025 01:59
Copy link
Contributor

@sle-c sle-c left a comment

Choose a reason for hiding this comment

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

CI builds failed on Ruby v2x which we don't support anymore. I've confirmed this change works with the current test suite in Shopify core.

@sle-c sle-c merged commit bd2cbde into 9_stable Jan 6, 2025
3 of 13 checks passed
@sle-c sle-c deleted the active_resource_autoload branch January 6, 2025 20:11
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.

2 participants