-
Notifications
You must be signed in to change notification settings - Fork 52
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
before_action
like filters for inertia_share
#137
before_action
like filters for inertia_share
#137
Conversation
4742060
to
b998284
Compare
10d11e8
to
4e1babe
Compare
4e1babe
to
2309b58
Compare
I love the API. Especially the clear and helpful exceptions if you try write something that could lead to keyword collision. Last night when I looked at this, I worried about this as well:
I.e. would it be confusing for someone new if the API to kinda sorts resembles However, after thinking about it, I think keys like if/only/except/unless are generic enough that they don't "belong" to callbacks. The pattern here feels very much like a Rails pattern, rather than a Rails callbacks pattern. I'm in favor of adding. I would use this in existing apps. @BrandonShar can you strawman this? Are there any other potential downsides you can think of that we should consider? |
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.
I like it! I think this is a nice "papercut" style improvement that feels very in line with existing expectations for rails users.
Just two minor comments I'd prefer be addressed before we merge.
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.
Thanks for the back and forth!
This PR builds on the approach from #121 and aims to resolve the concerns raised in #115 (comment).
It introduces some complexity as we need to partially reimplement Rails callbacks to work with @ElMassimo's shared properties feature.
@bknoles, I'd like to get your opinion on this approach. Do you think it's worth it to continue refining this PR?
Supported options:
Caveat: