-
Notifications
You must be signed in to change notification settings - Fork 54
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
Refactor inertia share to use instance variables #111
Refactor inertia share to use instance variables #111
Conversation
Move code to InertiaRails::Controller and use `class_attribute` to manage definitions without affecting parent controller. This avoids resetting shared data (`InertiaRails.reset!`) on every request. Instead, the shared data is defined at class loading. This change is thread-safe by design, so the multi-threading-test is removed.
wow that was fast |
@bknoles i see the problem with class_attribute and i refactor with this idea in mind: #108 (comment). Basically using instance attributes and |
Want to review the code a bit more, but, as I mentioned in #115 , I'm leaning towards merging this! |
The tests that are failling in CI are bit flaky, i dont know if is a error in the test code or in the new controller implementation i can check better in this friday |
Great addition! Refactoring to stick with default Rails conventions by using instance variables to store |
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.
This looks good to me! I asked for a couple small changes plus to revert the multithreaded spec back to its original state (so we aren't changing the implementation and the spec at the same time).
Once we get those hammered out I'll get this merged in! Thanks for the good work and the patience + persistence!
Team support! I like it! 😀 |
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.
At long last, a merge! Thanks for sticking with it!
Released as part of |
Goal
This PR refactors how
inertia_share
is storage inside the request lifecycle.Solution
The solution was to sticky with default rails conventions, using only instance variable to storage inertia_share data and using inertia_share as a wrapper to rails before_action
Future improves
only
,expect
features from before_action.