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

💡 RFC: client:media should be composable with :load, :visible, and :idle #877

Closed
1 of 3 tasks
jasikpark opened this issue Jul 27, 2021 · 20 comments
Closed
1 of 3 tasks

Comments

@jasikpark
Copy link
Contributor

Background & Motivation

I want to be able to load certain components only at a certain breakpoint while also selecting when it loads, which currently client:media does not allow, as it defaults to eagerly loading.

Proposed Solution

Possible solutions

<Component client:visible:media="(min-width: 50em)"/>

This could potentially compose similar to tailwindcss directives.

Alternatives considered

Risks, downsides, and/or tradeoffs

Open Questions

Detailed Design

No response

Help make it happen!

  • I am willing to submit a PR to implement this change.
  • I am willing to submit a PR to implement this change, but would need some guidance.
  • I am not willing to submit a PR to implement this change.
@FredKSchott
Copy link
Member

Possible syntax:

1.
<Component client:visible:media="(min-width: 50em)"/>
2.
<Component client:visible client:media="(min-width: 50em)"/>

Use-case:

  • on docs site, by moving to client:media we lost the ability to use client:idle (forces client:load behavior).
  • We'd like to add that back.

RFC Discussion:

  • These would currently be treated as and
  • could this open the door to custom directives?

RFC Call:

  • No objection to moving forward with this conceptually
  • syntax still tbd

@tony-sull
Copy link
Contributor

I think the <Component client:visible client:media="(min-width: 50em)"/> may be the best bet in case we add another hydrator that needs to take in a value like :media

@jasikpark
Copy link
Contributor Author

True. <Component client:media="(min-width: 50em)":visible/> doesn't look amazing... though maybe we could just allow both, with the chaining as a shorthand for the common case?

@eyelidlessness
Copy link
Contributor

The answer for the syntax is in the issue title :)

It’s not composable if you have to use a single attribute. Separate attributes with their own respective semantics allow composition, and can be applied with a well-understood evaluation order (inside out).

Probably with some special casing internally to work around issues already present (various nesting scenarios).

@matthewp
Copy link
Contributor

I agree with @eyelidlessness that separate attributes are needed. I think we might not need special syntax here, if there are multiple attributes then use this combined behavior.

Just as an implementation note that how hydrators work currently it's not an easy change to make. Probably this part will need a major refactor to enable this feature.

@eyelidlessness
Copy link
Contributor

Given hydration is already async (it depends on at least one import), couldn't you conceivably wrap each hydration method in a Promise that resolves when that hydration should occur, and await Promise.all(hydrationPromises)?

@FredKSchott
Copy link
Member

I'll play devils advocate: why do we need separate attributes? Do we have any use-cases outlined? Could we instead make them default to idle?

@matthewp
Copy link
Contributor

matthewp commented Aug 24, 2021

@FredKSchott some hydrators like media need a value. In the RFC call we discussed a potential new hydrator that also has a value (client:interaction). If you wanted to compose these two how would you have a value for each of them?

<Component client:media="(min-width: 800px)" client:interaction="focus" />

<!-- If combined? -->
<Component client:media:interaction="(min-width: 800px)|focus"

@FredKSchott
Copy link
Member

My point was: what if we intentionally say that we don't support composing two hydrator directives?

Given how complex this might be to refactor, do we have a clear use-case where a user needs this feature?

@matthewp
Copy link
Contributor

@FredKSchott Your comment here has uses cases: #877 (comment)

I think I'm still not understanding you though, as I currently read your comment to mean to not implement this RFC at all...

@FredKSchott
Copy link
Member

I currently read your comment to mean to not implement this RFC at all...

Yup! Hence the "devils advocate" preface 😈 . The use-case that I outlined above is a pretty classic "nice to have". Given the complexity to implement this RFC, is it worth the effort & complexity?

@matthewp
Copy link
Contributor

Oh ok, so I was understanding!

I would think that "is it worth it" would be quite valid if we are talking new syntax, but if we are not then I side on it being worth implementing. Not a priority for the core team, but I have no objection to someone else taking it on.

If the complexity question comes from my comment, I didn't mean to scare off people. Just wanted to note that the current implementation assumes 1-to-1 so it would have to be refactored. It's the usual complexity when moving from 1 to N things, so more effort than a quick lunch time PR. Or I could be wrong and this is super easy to do!

@eyelidlessness
Copy link
Contributor

So, rather than a devil’s advocate argument… what if this is the wrong solution to a problem, and reframing the problem may lead to a better solution?

Currently Astro addresses a limited but growing range of hydration options. Composition is just one of many potential ways people might want to expand that selection. At a certain point expansion becomes so general you arrive at… the general solution: hydrate:on={ arbitraryFunction }. Which I know has been discussed, and I’d be shocked if it isn’t inevitable.

Obviously that’s less ergonomic than hydrate:idle etc. But those could also be shorthand for the general case, and the longhand functions they reference could also be explicit Astro exports. And… there's your composition! Functions are inherently composable.

I still think the nesting case should be addressed, last I checked a hydrate:load inside a hydrate:idle never hydrates (or possibly gets replaced?) I’d hope the broad strokes Promise.all approach I mentioned above would help to resolve that without too much complexity.

@jasikpark
Copy link
Contributor Author

jasikpark commented Aug 25, 2021

I'd be in favor of a simple "if any of these are true, hydrate" composing syntax as I suggested, plus an API for defining functions for loading directives that would handle more complex cases.

Would it be Promise.all or Promise.any?

@eyelidlessness
Copy link
Contributor

Okay now I'm all in on "this should be an arbitrary function". I was interpreting composition as .every, not .some. And I can definitely see that distinction being confusing as heck.

@jasikpark
Copy link
Contributor Author

Okay now I'm all in on "this should be an arbitrary function". I was interpreting composition as .every, not .some. And I can definitely see that distinction being confusing as heck.

Actually, I've forgotten my original ask: that it is .every since I want :media && :visible oops

<Component client:load:and:media="(min-width: 700px)"/>

😨

@jasikpark
Copy link
Contributor Author

jasikpark commented Aug 25, 2021

Wait a second... shouldn't client:visible cover client:media for the general usecase? since you're going to be setting display: none on it????

@matthewp
Copy link
Contributor

The idea of implementing your own hydrator has come up a few times, which I think is what you're suggesting @eyelidlessness. This is indeed a more powerful solution. We probably need an RFC to talk about a specific solution to that.

@eyelidlessness
Copy link
Contributor

@matthewp yeah it could be another RFC, but I'm suggesting it may also be the appropriate solution to this one.

@FredKSchott
Copy link
Member

Hey everyone! Our current RFC process is beginning to break down at this size, with over 50 open RFCs currently in the "discussing" stage. A growing community is a great problem to have, but our ability to give you RFC feedback has suffered as a result. In an effort to improve our RFC process, we are making some changes to better organize things.

From now on, all RFCs will live in a standalone repo: https://github.com/withastro/rfcs

This allows us to do three things: 1) Use threaded discussions for high-level ideas and improvements, without necessarily requiring an implementation for every idea. 2) Improve the quality of our RFC template and the speed/quality of all feedback. 3) Support inline comments and explicit approvals on RFCs, via a new Pull Request review process.

We hope that this new process leads to better RFC weekly calls and faster feedback on your RFCs from maintainers. More detail can be found in the new RFC repo README.


We can't automatically convert this issue to an RFC in the new repo because new RFC template is more detailed that this one. But, you can still continue this discussion in the new repo by creating a new Discussion in the RFC repo and copy-and-pasting this post (and any relevant follow-up comments) into it. Discussions are available for high-level ideas and suggestions without the requirement of a full implementation proposal.

Then, when you are ready to propose (or re-propose) an implementation for feedback and approval, you can create a new RFC using the new RFC template. More detail about how to do this can be found in the new RFC repo README.

Thanks for your patience as we attempt to improve things for both authors and reviewers. If you have any questions, don't hesitate to reach out on Discord. https://astro.build/chat

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

No branches or pull requests

5 participants