Skip to content

Some enhancements/fixes to the Lit filter from testing #141

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

Merged
merged 5 commits into from
Oct 11, 2021

Conversation

jaredcwhite
Copy link
Member

@jaredcwhite jaredcwhite commented Oct 1, 2021

I spent some time with the latest Lit filter goodies. Very cool @rubys!

I came across a few gotchas while trying it out, so this PR will:

  • Allow snake case custom_element in addition to customElement
  • Process the render function even when it contains multiple statements
  • Fix a properties vs styles typo for <es2022
  • Automatically add the html tagged literal return when necessary for any method, not just render
  • Ensure any ivars prefixed with _ will not turn into reactive properties (if you truly need to maintain private internal state).

There's one outstanding bug I saw which is that any ivar set at the class level (like @styles for example) is turning into a reactive property. I suspect it's just a matter of ensuring the walk algo doesn't touch ivars unless they're inside instance method definitions, but I didn't investigate further.

TODO:

  • Add documentation

@jaredcwhite
Copy link
Member Author

On a side note, I'm pretty sure this filter eliminates the need for the syntactic sugar I'd previously worked on for CrystallineElement, so I can standardize on just inheriting from LitElement directly and then add my two controllers manually. Awesome sauce! (ACYMI, they're controllers which essentially replicate Stimulus light DOM functionality for Lit elements, aka actions and targets.)

@rubys
Copy link
Member

rubys commented Oct 1, 2021

Update https://github.com/ruby2js/ruby2js/blob/master/docs/src/_docs/filters/lit.md too?

There's one outstanding bug I saw which is that any ivar set at the class level (like @Styles for example) is turning into a reactive property.

@styles now feels wrong, it probably would be better as @@styles. But there is another bug there that I will address in that @@properties it isn't merged correctly with other inferred properties. I also want to make that a deep merge so that one can, for example, declare a property as attribute: false and allow inferencing to infer the type.

I also plan to update the Rails example to the latest Lit example (it currently is based on the polymer LitElement example).

P.S. As always, feel free to directly commit.

P.P.S. I feel uneasy about "Automatically add the html tagged literal return when necessary for any method, not just render", but real world usage is the best way to figure out if this is problematic.

@jaredcwhite
Copy link
Member Author

I feel uneasy about "Automatically add the html tagged literal return when necessary for any method, not just render", but real world usage is the best way to figure out if this is problematic.

Well, perhaps that's a step too far in the magic department, but if so, I think we'd want to back out of updating render as well (aka you still need to prefix the string with html). I feel weird about having render be special among methods. (the use case here is I will often break my render method up by putting bits of templates in other methods in a "partials" type of approach)

@rubys
Copy link
Member

rubys commented Oct 1, 2021

partials is definitely a valid use case -- just document it. Not having to annotate strings as HTML is definitely magic, but very convenient.

While it made me feel uneasy, the more I think about it, the case I would be concerned about is rare... a return statement that returns a constant string (possibly with interpolation), where the first character is a less than sign.

That being said, ideally there would be a way to indicate that a string is to be returned? Perhaps as simple as enclosing the expression in parenthesis? I believe that is enough to defeat the current filter logic, so perhaps document it and count it as a win?

@jaredcwhite
Copy link
Member Author

@rubys Looks like the parenthesis doesn't short-circuit the logic, but assigning the string to a variable first and returning that does, so probably just need to document that.

@rubys
Copy link
Member

rubys commented Oct 5, 2021

Any objections to this pull request being merged? I was in the process of making a number of upgrades to the Lit filter (and you, in fact, completed a number of these changes for me - thanks!) and I would like to resume making fixes/upgrades and then make a release. At the moment, lit is broken on master which is what is preventing a release containing a CLI...

@jaredcwhite
Copy link
Member Author

jaredcwhite commented Oct 5, 2021

@rubys Yeah I was hoping to get to the docs updates but haven't had time yet. Features should be solid now, so let's merge and I'll work on docs soon.

@render
Copy link

render bot commented Oct 11, 2021

@jaredcwhite
Copy link
Member Author

jaredcwhite commented Oct 11, 2021

I noticed website builds weren't working on Render anymore, so I upgraded some dependencies (Bridgetown, the quick search plugin which now uses Lit 2, and Shoelace). I also realized I'd forgotten to turn on PR previews, so now we have those as well. =)

@jaredcwhite jaredcwhite merged commit 188fa71 into master Oct 11, 2021
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