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

Active Storage: docs on lazy vs immediate loading #42495

Merged
merged 1 commit into from
Jun 16, 2021
Merged

Conversation

ghiculescu
Copy link
Member

Follow up to #42477 (comment)

@rails-bot rails-bot bot added the docs label Jun 15, 2021
@ghiculescu
Copy link
Member Author

@wbharding @RSO I used some of your examples from #40842 for this. Can you please have a look and see if they make sense?

@pixeltrix FYI

@@ -687,6 +687,38 @@ previewable files. You can also call these methods directly.
[`representable?`]: https://api.rubyonrails.org/classes/ActiveStorage/Blob/Representable.html#method-i-representable-3F
[`representation`]: https://api.rubyonrails.org/classes/ActiveStorage/Blob/Representable.html#method-i-representation

### Lazy vs Immediate Loading
Copy link
Member

@p8 p8 Jun 16, 2021

Choose a reason for hiding this comment

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

I think loading is a bit ambiguous here. Is it about lazily processing the images or about redirecting the back to the app first?
Maybe:

Suggested change
### Lazy vs Immediate Loading
### Lazy vs Immediate Processing

Copy link
Member Author

Choose a reason for hiding this comment

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

I think "lazy loading" is a much more commonly used term to describe the pattern. So I would rather stick with that, in case a user is confused by the terminology and googles it.

image

vs

image

Copy link
Member

Choose a reason for hiding this comment

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

Ok, but in this case it's mostly about deferring the processing to another request right? At least the first time, when blobs haven't been processed...

guides/source/active_storage_overview.md Outdated Show resolved Hide resolved
guides/source/active_storage_overview.md Outdated Show resolved Hide resolved
guides/source/active_storage_overview.md Outdated Show resolved Hide resolved

```ruby
user.avatars.with_all_variant_records.each do |file|
image_tag file.representation(resize_to_limit: [100, 100]).processed.url
Copy link
Member

Choose a reason for hiding this comment

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

This will process all N files on the first access right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Member

@p8 p8 Jun 17, 2021

Choose a reason for hiding this comment

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

Should we maybe at a warning that it can make pages slow?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess maybe this isn't direct enough...

image

I could do followup.

Copy link
Member

Choose a reason for hiding this comment

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

This also bypasses HTTP caching, as it will create a new temporary url each time it's called.
So you need to either cache the url or users will download the file every time they access the page.

Co-authored-by: Petrik de Heus <petrik@deheus.net>
@zzak zzak merged commit c204eec into rails:main Jun 16, 2021
@ghiculescu ghiculescu deleted the patch-1 branch June 16, 2021 23:24
@RSO
Copy link

RSO commented Jun 17, 2021

Late to the party, but looks good to me. A small nitpick would be that I don't think users will have many avatars typically, but that shouldn't be an issue in understanding what this is about.

Side note: Is .representations something new? It seems like we're mixing variant and representation as a name for the same thing.

@p8
Copy link
Member

p8 commented Jun 17, 2021

@RSO It's a wrapper so you don't have to think about if you need variant or preview.

Returns an ActiveStorage::Preview for a previewable blob or an ActiveStorage::Variant for a variable image blob.
https://api.rubyonrails.org/classes/ActiveStorage/Blob/Representable.html#method-i-representation

@p8
Copy link
Member

p8 commented Jun 17, 2021

@RSO maybe we should use message.images instead of user.avatars, as message.images is used in a lot of examples, and users.avatars is used only once. Could you make a PR?

zzak added a commit that referenced this pull request Jun 18, 2021
@zzak
Copy link
Member

zzak commented Jun 18, 2021

Fixed in 9c091b4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants