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

Custom Client Directives #3241

Closed
bluwy opened this issue May 12, 2023 · 11 comments · Fixed by #3253
Closed

Custom Client Directives #3241

bluwy opened this issue May 12, 2023 · 11 comments · Fixed by #3253
Labels
add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!) minor-release For the next minor release; in the milestone, "merge queue" when approved by Sarah!

Comments

@bluwy
Copy link
Member

bluwy commented May 12, 2023

Summary

Support custom client: directives that can be added by integrations. Enabled with experimental.customClientDirectives option

Todoc

  1. experimental.customClientDirectives option
  2. Integration astro:config:setup addClientDirective() API
  3. How to provide typings for custom client: in .astro files
@sarah11918 sarah11918 added add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!) labels May 12, 2023
@sarah11918
Copy link
Member

Thanks for this, @bluwy !

Initial questions:

  • if I think about what people need to do in order to use this, do they need to both set an experimental flag and install the package?
  • This seems more similar to our RSS package (which we do call a package, not an integration). So, am I correct that we should be referring to this as a package, not an integration? Integrations show up on this page in docs, with a logo.
  • Either way, package or integration, it should have a README for the package. For integrations, we pull the content into docs automatically. For packages, we document manually. Since this is experimental, I think it makes sense to use the package's own README as its docs, and we can link to there from the Template Directives reference page. Something like:

Experimental: you can implement custom client: directives to provide greater control for when client-side JS is loaded and executed. See the @astro/click-directive package for more details.

Those are the initial things I'm thinking about here!

@matthewp
Copy link
Contributor

@sarah11918 we are not including any custom directives with this release (correct me if I'm wrong @bluwy).

So this is a bit tricky right now as the end-user has to enable a flag and install some packages, but those packages do not exist yet.

Maybe it makes sense to focus the documentation on the audience of people who want to build custom directives?

This is most similar to our concept of framework renderers. The audience for someone building their own renderer is different from the audience that wants to use a renderer built by someone else.

@sarah11918
Copy link
Member

@matthewp Yup, I gathered that! My thinking is someone on the reference page reading about Template Directives gets a link to know that there's a place they can read about creating their own custom directives, then the package README is as technical as you want for builders. But, at least this way someone looking up the references in docs can see that there's a(n experimental) way to build one themselves.

@matthewp
Copy link
Contributor

Side question, would it be helpful to have an example in the examples/ folder for building a client directive? Or do you think documentation would be enable?

@sarah11918
Copy link
Member

Examples are always helpful! But I absolutely think documentation is enough for this release.

@bluwy
Copy link
Member Author

bluwy commented May 13, 2023

Yeah there will be no new directives and package/integrations added in this feature. What we do allow is new kinds of integrations that add custom client directives, which the user has to also opt-in with an experimental flag (to show they know what they're getting into).

@sarah11918
Copy link
Member

Yup, so to be clear, what I'm expecting is:

  1. As part of the withastro/astro feature PR, include:
  • a README.md file in the package itself. This is where you'll describe how to enable (experimental flag, install the package) and use the thing. You can use the RSS package README as a basic example.
  • an update to the Experimental Flags section of the astro types file that just literally mentions that this exists as an experimental flag. Use the other experimental flags as a basic example
  1. A Docs PR to our Template Directives reference page that adds a new section at the bottom of the Client Directives section (after Client:only) with the heading something like "Custom Directives" and just mentions a line like:

Experimental: It is also possible to build your own custom directives using @astrojs/package-name.

I'll happily review and edit all of these, so just look at existing examples and write what you feel makes sense!

@bluwy
Copy link
Member Author

bluwy commented May 15, 2023

Re no1, there's no new package or integrations introduced for this feature, so I'm a bit confused. We're only introducing the experimental.customClientDirectives flag, and the addClientDirective() API for integrations.

Re no2, that makes sense to me (besides the @astrojs/package-name part). For the addClientDirective() API, the integrations API page seems like a good spot.

@sarah11918
Copy link
Member

@bluwy Ah, I was going off of Matthew's comment that some packages would need to be installed AND a user flag. I thought the user would have to install a package in order to write custom directives. But, I think the packages referred to are the new (not yet existing?) custom directives? So, you'd install each new directive you wanted that Astro has created, in addition to being able to write your own?

If yes, then I agree with you! So:

  1. Just the experimental flag in the astro types file (which will show up in the configuration reference page)
    And the docs PR can include:
  2. the integrations API page- add content about addClientDirective
  3. the Template Directives page - a note to say that you can build your own client directives (experimentally)... unless you don't want people doing this yet. 😄

@bluwy
Copy link
Member Author

bluwy commented May 15, 2023

But, I think the packages referred to are the new (not yet existing?) custom directives? So, you'd install each new directive you wanted that Astro has created, in addition to being able to write your own?

Yes 👍 the packages we're referring to are future third-party packages/integrations that would use the new addClientDirective API. The experimental flag is added as a guard for users of the packages to say "I know what I'm doing". Ideally these third-party packages' README would point users that they need to turn on the flag to use their integration.

But of course the users can also create their own directive integration locally too, similarly to the usual local integration workflow (My PR has a test setup example).

Hope that clears things up! I agree with your 3 points too. I'll make sure my core PR gets no1 ready too.

@sarah11918
Copy link
Member

Yup, perfect, thanks @bluwy!

@sarah11918 sarah11918 added the minor-release For the next minor release; in the milestone, "merge queue" when approved by Sarah! label Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!) minor-release For the next minor release; in the milestone, "merge queue" when approved by Sarah!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants