-
Notifications
You must be signed in to change notification settings - Fork 66
Add specs from config refactor #139
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
Changes from all commits
ebddc88
93a0034
e25dc6b
4d1de65
4453e8f
a887fcc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ Gem::Specification.new do |spec| | |
spec.name = "inertia_rails" | ||
spec.version = InertiaRails::VERSION | ||
spec.authors = ["Brian Knoles", "Brandon Shar", "Eugene Granovsky"] | ||
spec.email = ["brain@bellawatt.com", "[email protected]", "[email protected]"] | ||
spec.email = ["brian@bellawatt.com", "[email protected]", "[email protected]"] | ||
|
||
spec.summary = %q{Inertia adapter for Rails} | ||
spec.homepage = "https://github.com/inertiajs/inertia-rails" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
class InertiaConditionalSharingController < ApplicationController | ||
before_action :conditionally_share_a_prop, only: :show_with_a_problem | ||
|
||
inertia_share normal_shared_prop: 1 | ||
|
||
inertia_share do | ||
|
@@ -16,4 +18,16 @@ def show | |
show_only_prop: 1, | ||
} | ||
end | ||
|
||
def show_with_a_problem | ||
render inertia: 'EmptyTestComponent', props: { | ||
show_only_prop: 1, | ||
} | ||
end | ||
|
||
protected | ||
|
||
def conditionally_share_a_prop | ||
self.class.inertia_share incorrectly_conditionally_shared_prop: 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. genuine question: Why are we explicitly testing that this fails as opposed to just not testing it if it's undocumented behavior? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I first wrote a variation of this test, it didn't fail. It actually passed. Before #121 , this code would not conditionally share that prop. Because the inertia_share store is a class instance variable, it would be included on all subsequent requests after the first time it ran. It's not very elegant code, but I think the use case is valid. If I can share data per-controller, why not per action? I wrote the original spec just to document the behavior. However, @ElMassimo 's refactor actually prevents this from occurring at all because it freezes the shared data structure once the controller class is loaded into memory. And that is (part of) the intended behavior of his change. So, the spec now documents/ensures you can't even try the contrived workaround for per-action data sharing. To me, this is a nice guardrail that fails loudly, and since I was surprised by the new behavior when I added the spec back, I think it's worth keeping! |
||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
class InertiaShareTestController < ApplicationController | ||
inertia_share name: 'Brandon' | ||
inertia_share sport: -> { 'hockey' } | ||
inertia_share({a_hash: 'also works'}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
inertia_share do | ||
{ | ||
position: 'center', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,30 @@ | ||
# Specs as documentation. Per-action shared data isn't explicity supported, | ||
# but it can be done by referencing the action name in an inertia_share block. | ||
RSpec.describe "conditionally shared data in a controller", type: :request do | ||
context "when there is conditional data inside inertia_share" do | ||
it "does not leak data between requests" do | ||
get conditional_share_index_path, headers: {'X-Inertia' => true} | ||
expect(JSON.parse(response.body)['props'].deep_symbolize_keys).to eq({ | ||
index_only_prop: 1, | ||
normal_shared_prop: 1, | ||
}) | ||
|
||
# NOTE: we actually have to run the show action twice since the new implementation | ||
# sets up a before_action within a before_action to share the data. | ||
# In effect, that means that the shared data isn't rendered until the second time the action is run. | ||
get conditional_share_show_path, headers: {'X-Inertia' => true} | ||
context "when there is data inside inertia_share only applicable to a single action" do | ||
it "does not leak the data between requests" do | ||
get conditional_share_show_path, headers: {'X-Inertia' => true} | ||
expect(JSON.parse(response.body)['props'].deep_symbolize_keys).to eq({ | ||
normal_shared_prop: 1, | ||
show_only_prop: 1, | ||
conditionally_shared_show_prop: 1, | ||
}) | ||
normal_shared_prop: 1, | ||
show_only_prop: 1, | ||
conditionally_shared_show_prop: 1, | ||
}) | ||
|
||
get conditional_share_index_path, headers: {'X-Inertia' => true} | ||
expect(JSON.parse(response.body)['props'].deep_symbolize_keys).to eq({ | ||
index_only_prop: 1, | ||
normal_shared_prop: 1, | ||
}) | ||
expect(JSON.parse(response.body)['props'].deep_symbolize_keys).not_to include({ | ||
conditionally_shared_show_prop: 1, | ||
}) | ||
end | ||
end | ||
|
||
context "when there is conditional data shared via before_action" do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Freezing the shared data prevents the |
||
it "raises an error because it is frozen" do | ||
# InertiaSharedData isn't frozen until after the first time it's accessed. | ||
InertiaConditionalSharingController.send(:_inertia_shared_data) | ||
|
||
expect { | ||
get conditional_share_show_with_a_problem_path, headers: {'X-Inertia' => true} | ||
}.to raise_error(FrozenError) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does anyone by chance know why Rails 7.0 and lower doesn't raise an error here while Rails 7.1 does? My guess is that maybe there was a change in the way Rails loads the code in 7.1, and previous versions don't actually load/freeze the data in the controller before this line gets called? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use Rails 7.1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, that was part of it but there was a second subtler bug. Because the shared data is frozen the first time you access the shared data, if the first request to a controller has one of these (contrived) conditionally shared data examples, then it won't raise an error. So, in the test suite, the failure (to raise a FrozenArray exception) was intermittent, depending on which spec in that file ran first. It's an edge case of an edge case, so I don't feel we need to prevent it; and I still like having a spec to document the frozen data behavior. In any case, thanks for #140 , it gave me a headstart tonight once I got back to looking at this one! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, good catch. What do you think about marking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, that might be cleaner. I'm not super worried about it either way now. It'll raise the FrozenArray error the second time you try to call the conditional action, so the failure I was seeing was just one of those test suite not quite matching reality quirks. The per-action use case isn't actually officially supported at the moment, so we're being helpfully defensive enough already, IMO. |
||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -154,4 +154,12 @@ | |
end | ||
end | ||
end | ||
|
||
describe 'a non existent route' do | ||
it 'raises a 404 exception' do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Arguably this is just testing Rails instead of InertiaRails, but I wrote this to reproduce a possible bug that @skryukov found in the middleware, so I'm leaving it in. |
||
expect { | ||
get '/non_existent_route', headers: { 'X-Inertia' => true } | ||
}.to raise_error(ActionController::RoutingError, /No route matches/) | ||
end | ||
end | ||
end |
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 was a cool email, though 😄
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.
Maybe it should be my cooler alter ago 😅!