Skip to content
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

Allow omitting the component name with props on render #199

Merged
merged 3 commits into from
Mar 11, 2025

Conversation

skryukov
Copy link
Contributor

@skryukov skryukov commented Mar 1, 2025

This PR improves the automatic component name functionality, with these changes we can now render a page like this:

class UsersController < ApplicationController
  def index
    render inertia: { users: User.all.as_json(...) } # => renders pages/users/index.tsx with {users} props
  end
end

@skryukov
Copy link
Contributor Author

skryukov commented Mar 1, 2025

Linter fails, but I fixed it in other PRs, so will just wait 😂

@BrandonShar
Copy link
Collaborator

Oh that's nice. Much cleaner than the useless inertia: true response. So this is basically for users who want the automatic routes, but still want to be explicit about props?

@skryukov
Copy link
Contributor Author

skryukov commented Mar 1, 2025

Yup, used that a lot with inertia_config default_render: true

Copy link
Collaborator

@BrandonShar BrandonShar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome, I'll get some things merged!

Comment on lines 10 to 13
if component.is_a?(Hash) && options[:props].nil?
options[:props] = component
component = true
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason we couldn't/shouldn't put this logic in the renderer file? Its initialize method is already figuring out how to handle the (overloaded) component argument. Might be nice for Future Us to have the argument munging logic in one place.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check for this condition and raise a helpful error?

render inertia: { users: User.all }, props: { just_kidding: 'about that users prop!' }

As it is I think this would fail as a white screen of death on the frontend?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a check

@bknoles
Copy link
Collaborator

bknoles commented Mar 3, 2025

I think you could do this already to get automatic component paths with explicit props, right?

render inertia: true, props: { some: 'stuff' }

That said, this does look nicer:

render inertia: { some: 'stuff' }

I don't love that this component argument could now mean 3 things: 1) a component path string, 2) a hash of props, 3) our trusty magic TrueClass to tell the controller to go figure out the path on its own, please.

What if we deprecate the true option, replacing it (in practice) with:

render inertia: {}

in the case where you want the automatic path resolution but don't need to send props.

@skryukov skryukov force-pushed the allow-passing-props branch from b2aab22 to 2b738cd Compare March 4, 2025 09:54
@skryukov skryukov force-pushed the allow-passing-props branch from 2b738cd to 380f9dc Compare March 4, 2025 09:57
@skryukov
Copy link
Contributor Author

@bknoles is there anything holding us from merging this one?

@bknoles
Copy link
Collaborator

bknoles commented Mar 11, 2025

Sorry for the delay! I was on the fence on two things:

  1. (warning: this is pedantic) i'd like to rename the component argument to component_or_data just for future clarity
  2. I'd like to deprecate render inertia: true in favor of render inertia: {}, with an actual deprecation warning (instead of pedantically suggesting component_or_data_or_magic_true 😝)

Coming back to it today, neither is important enough to me to delay merging

@bknoles bknoles merged commit 04d5a3b into inertiajs:master Mar 11, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants