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

Allow to load associations asynchronously #26

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

casperisfine
Copy link

Opening here rather than upstream for now, because this is just exploratory work.

Notes:

  • Supporting the happy path for belongs_to / has_one / has_many is quite easy.
  • Not sure we can support :through relations, but these are rare so not a priority.
  • Implementation wise, this duplicate quite a few codepaths. At some point we may want to always wrap results in promises, even when doing sync queries to reduce cyclomatic complexity, hence code paths to tests.

FYI: @hcmaATshopify @rafaelfranca


fixtures :companies, :accounts

self.use_transactional_tests = false
Copy link
Author

Choose a reason for hiding this comment

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

Note to self. With the recent refactor in connection pool / pinned connections, we should actually be able to properly simulate async queries with transactional tests. We just need a lock around the connection.

Choose a reason for hiding this comment

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

@casperisfine We are interested in this - would we have to do anything in the tests themselves?

Copy link

@hcmaATshopify hcmaATshopify left a comment

Choose a reason for hiding this comment

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

@byroot: two questions:

  • I am assuming that the changes we will make to the production code will be of the form <object>.association(:<associated_object>).async_load_target, correct?
  • What will be the best way for us to get this change for testing in the Core code?

@casperisfine
Copy link
Author

casperisfine commented Apr 12, 2024

will be of the form <object>.association(:<associated_object>).async_load_target, correct?

For now yes, this is just to test the internal capability, this isn't likely to be the final "end user API". It's more likely to be something like:

post.load_async(:comments, tags, :author)

I'm still thinking about it.

What will be the best way for us to get this change for testing in the Core code?

Depends what you need. If you just want to test locally or on CI, you can just edit the Gemfile, to replace:

gem "rails", github: "rails/rails", branch: "main"

by:

gem "rails", github: "https://github.com/Shopify/rails/pull/26"

Copy link

@hcmaATshopify hcmaATshopify left a comment

Choose a reason for hiding this comment

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

@casperisfine thanks for this! It LGTM. When do you intend to release this?


fixtures :companies, :accounts

self.use_transactional_tests = false

Choose a reason for hiding this comment

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

@casperisfine We are interested in this - would we have to do anything in the tests themselves?

first_firm = companies(:first_firm)

promise = client.load_async(:firm)
wait_for_async_query
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to wait? Should not client.firm wait already?

Copy link
Author

Choose a reason for hiding this comment

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

We don't need to, it's only for testing purpose to ensure the query is actually executed in the background.

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.

5 participants