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

[ISSUE-408] Lazy Network Calls on Collections #409

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jlurena
Copy link

@jlurena jlurena commented Nov 23, 2024

Summary

This is for #408 .

It is a breaking change

ActiveResource allows you to chain where methods such as

result = MyActiveResourceModel.where(first_name: "John").where(last_name: "Doe")

This improves it so that network calls are only performed when resources are accessed.

With this PR

# This makes 0 API call
result = MyActiveResourceModel.where(first_name: "John").where(last_name: "Doe")

# This makes 1 API call
result.to_a

# This makes 1 API call
result.any?

@@ -150,7 +150,7 @@ def defines_has_many_finder_method(reflection)
elsif !new_record?
instance_variable_set(ivar_name, reflection.klass.find(:all, params: { "#{self.class.element_name}_id": self.id }))
else
instance_variable_set(ivar_name, self.class.collection_parser.new)
instance_variable_set(ivar_name, reflection.klass.find(:all))
Copy link
Author

Choose a reason for hiding this comment

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

Because this is now lazy, no network call is actually made.

Comment on lines +800 to +804
def instantiate_record(record, prefix_options = {})
new(record, true).tap do |resource|
resource.prefix_options = prefix_options
end
end
Copy link
Author

Choose a reason for hiding this comment

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

Changed from private to public

# Swallowing ResourceNotFound exceptions and return nil - as per
# ActiveRecord.
nil
collection_parser.new([], options[:from]).tap do |parser|
Copy link
Author

Choose a reason for hiding this comment

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

Network call now gets made within the ActiveResource::Collection class, so this is simply initializing it.

@@ -1140,13 +1156,6 @@ def instantiate_collection(collection, original_params = {}, prefix_options = {}
end.collect! { |record| instantiate_record(record, prefix_options) }
end

def instantiate_record(record, prefix_options = {})
Copy link
Author

Choose a reason for hiding this comment

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

Made public

Comment on lines -12 to -18
def test_collection_respond_to_collect!
assert @collection.respond_to?(:collect!)
end

def test_collection_respond_to_map!
assert @collection.respond_to?(:map!)
end
Copy link
Author

Choose a reason for hiding this comment

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

these methods were removed

Comment on lines -36 to -47
def test_collect_bang_modifies_elements
elements = %w(a b c)
@collection.elements = elements
results = @collection.collect! { |i| i + "!" }
assert_equal results.to_a, elements.collect! { |i| i + "!" }
end

def test_collect_bang_returns_collection
@collection.elements = %w(a)
results = @collection.collect! { |i| i + "!" }
assert_kind_of ActiveResource::Collection, results
end
Copy link
Author

Choose a reason for hiding this comment

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

these methods were removed

@jlurena jlurena marked this pull request as draft November 24, 2024 13:08
Similar to Rails ORM, check if request was made and re-use that instead of
making another http call.
@jlurena jlurena marked this pull request as ready for review November 24, 2024 15:20
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.

1 participant