-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add activestorage.rbi #108
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the index?
Various methods were not being handled correctly (e.g. `variant?`) for Rails models that were using ActiveStorage. This fixes many of those issues.
And update index.json as well.
7452621
to
0965278
Compare
Updated index.json and I think I've got the checks passing now, after running them locally. |
"rails" | ||
], | ||
"requires": [ | ||
"rails", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did it fail without rails
dependency and require?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not. Active Storage doesn't depend on Rails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since ActiveStorage::Blob/ActiveStorage::Attachment are technically models, it needs to load the ActiveStorage engine, and the engine failed to load until rails was added as a dependency. Is there a smaller dependency I can use to get the engine to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem seems to be there is a require "rails"
dependency in ActiveStorage, but it's not declared as a dependency in the gemspec. Same goes for Action Cable, Action Mailbox and Action Text.
"rails" | ||
], | ||
"requires": [ | ||
"rails", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not. Active Storage doesn't depend on Rails.
"dependencies": [ | ||
"rails" | ||
], | ||
"requires": [ | ||
"rails", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My previous comment was not correct, this work locally without upstream changes:
"dependencies": [ | |
"rails" | |
], | |
"requires": [ | |
"rails", | |
"dependencies": [ | |
"railties" | |
], | |
"requires": [ |
Rails contains a single file and dependencies on all gems (including Railties), Railties has what we actually need.
Type of Change
Changes
Various methods were not being handled correctly (e.g.
variant
) for Rails models that were using ActiveStorage. This fixes many of those issues.