Refactor route and namespace combination logic #926
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The current implementation of route and namespace combinations uses
@target_class.combined_*
accessors that inadvertently trigger hash copies due totransform_values
. This leads to inefficiencies and unexpected behavior where changes to supposedly original hashes do not persist.Solution
Shortly
The proposed refactor avoids using
@target_class.combined_*
until the final step, instead using local variables to accumulate and combine routes and namespaces directly. This change prevents premature hash copying and streamlines the combination process, ensuring that changes are properly reflected and improving overall performance.TLTR;
All
@target_class.combined_*
calls go through theadd_setup
method of thegrape
gem (caused byoverride_all_methods!
). In the depths of theadd_setup
method it does:The
transform_values
makes a copy of the original hash object and returns it. This leads to unexpected behavior in cases such as:Where expected flow:
@target_class.combined_routes
returns an original hash.resource
key on the original hash.combined_routes
is filled with a new key-value.But actual flow:
@target_class.combined_routes
creates a copy of the empty hashresource
keycombined_routes
is still an empty hash.