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

Rename templateview_template #630

Merged
merged 3 commits into from Jan 30, 2024
Merged

Rename templateview_template #630

merged 3 commits into from Jan 30, 2024

Conversation

joeldrapper
Copy link
Collaborator

This is a non-breaking rename that issues a warning whenever you define template.

@joeldrapper joeldrapper merged commit f0cdace into main Jan 30, 2024
17 checks passed
@joeldrapper joeldrapper deleted the view-template branch January 30, 2024 14:02
@davidalejandroaguilar
Copy link

Why was this needed?

@joeldrapper
Copy link
Collaborator Author

joeldrapper commented Mar 18, 2024

Because there’s an HTML tag <template>. In 2.0, the template method will render a <template> tag. In 1.10, you’ll be able to define a component template with either template or view_template, but using template is deprecated.

I spent a lot of time trying to avoid this change, but couldn’t find a way through. https://www.namingthings.org/p/magic-template-methods

@davidalejandroaguilar
Copy link

@joeldrapper Thanks for your response! Funny enough, I had just found that article myself right before you got back to me.

So, choosing to rename it to view_template turned out to be a better call than simply sticking with template and having that HTML tag be the only one called via template_tag, huh?

I get the urge to make that tweak as well – my "OCD" would have nudged me in the same direction. But I can't help thinking that using the shorter template instead of view_template would have been more up my alley.

@joeldrapper
Copy link
Collaborator Author

This decision wasn’t taken lightly. We spent hours discussing various options. We want it to be really easy for anyone who knows HTML to learn Phlex, and having to remember this one exception to the one-to-one mapping of methods to elements really harms that.

@davidalejandroaguilar
Copy link

Absolutely, that makes sense. I'm curious, do you see this change as a lasting one? Or do you think the compiler improvement you talked about in the article might allow us to revert back to using template in the future?

Also, I really appreciate you taking the time to break this down for me. I hope it didn't pull you away from your work for too long. It would be great if the reasoning behind changes like these could be included in the PR descriptions for those of us keeping an eye on the project.

@joeldrapper
Copy link
Collaborator Author

For now, the compiler is a really low priority because Phlex is so fast already. I expect view_template is here to stay now.

# article(class: "card", &block)
# end
def template
yield
end

def self.method_added(method_name)
if method_name == :template
Kernel.warn "Defining the `template` method on a Phlex component is deprecated and will be unsupported in Phlex 2.0. Please define `view_template` instead."
Copy link

Choose a reason for hiding this comment

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

💡 Seems I'm late to party with my story, since this check was already removed. I had to spent some time to find out which component emits this warning. If possible, add also some pointer like self.class to the output for future warning. 🙏 I was able to find out just by opening gem with bundle open and adding debug print in this method temporarily in the end.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey @simi, thanks for the feedback. Yes, we removed it from main, but it’s still in the latest version. I’ll try to remember to add more context to deprecation warnings in the future.

I hope we won’t need to do anymore of these going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants