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

inline in template boolean literals #385

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

titoBouzout
Copy link
Contributor

@titoBouzout titoBouzout commented Nov 28, 2024

This change, inlines in the template boolean literals. For example

<el attr={true}/> becomes <el attr/>
and
<el attr={false}/> becomes <el/>

Considerations:

  • this change skips doing this transformation for "custom elements", because as we are defaulting to properties I suspect something will break.
  • I do not think this is a breaking change, at least from my tests I didn't see anything that could cause problems.
  • this is related to an effort to fix the attributes vs properties problem

The most recent issue illustrating this props vs attr problem comes from #384 In there they are doing <video playsinline={true} />;, in production code currently:

  • client side works because compiles it to _el$.playsInline = true;
  • in ssr is inlined in the template
  • ssr with hydration fails because it tries to add the property with the lowercase name as _$setProperty(_el$, "playsinline", true);

With this change,

  • boolean attributes will always be inlined for the true case and skipped for the false case
  • once the pr with the types gets merged it will only accept "true" | boolean, so trying to do `"false"`` as a string should be a type error

I am hoping before solid 2.0 we can get rid of the hardcoded list of properties

Thanks!

…l attr/>`, `<el attr={false}/>` becomes `<el/>`
@ryansolid
Copy link
Owner

ryansolid commented Dec 10, 2024

The thing is the playsinline probably should be mapped properly anything that is denoted as a property should have translation for attributes. We just missed this one. That being said the other change is probably beneficial anyway.

Like to be clear. It is very intentional to use the properties for booleans and if we are to be able to set them as attributes as well then there is no way to get rid of the lookup.

Only question here is does this PR consider the fake booleans? Like things that need to be string "false" vs "true" where the application of just the property being there doesn't work. We have the constants list to differentiate between those.

@titoBouzout
Copy link
Contributor Author

Only question here is does this PR consider the fake booleans? Like things that need to be string "false" vs "true" where the application of just the property being there doesn't work. We have the constants list to differentiate between those.

Yes, this PR only affects literal true and false, it doesn't change enumerated attributes such "yes", "no", "true", "false", there are tests for such a case.

Like to be clear. It is very intentional to use the properties for booleans and if we are to be able to set them as attributes as well then there is no way to get rid of the lookup.

Can you please clarify what do you mean with its intentional?

@ryansolid
Copy link
Owner

Sorry for the delay of response. What I mean is that true boolean attributes.. like readonly or hidden etc.. are intended to be set as properties when dynamic in the client. Unlike say draggable which is set as an attribute.

I see this is a static case so we can inline them regardless.

But I was saying this sounds like a bug:

ssr with hydration fails because it tries to add the property with the lowercase name as _$setProperty(_el$, "playsinline", true);

It should be properly cased. Sure it is static here so it could be inlined but it should be set as _$setProperty(_el$, "playsInline", true); if it were dynamic.

I guess I can merge this as is.

@ryansolid ryansolid merged commit 8ed437c into ryansolid:main Jan 27, 2025
2 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.

2 participants