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.

Sign up for GitHub

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

Jump to bottom

Implement custom client directives #7074

Merged
merged 20 commits into from
May 17, 2023
Merged

Implement custom client directives #7074

merged 20 commits into from
May 17, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented May 12, 2023 •
edited
Loading

Changes

RFC: withastro/roadmap#583

  • Add experimental.customClientDirectives option.
  • Support integration astro:config:setup hook addClientDirective() API to add custom client directive.
  • Pass down clientDirectives map deep down to SSRResult to be rendered an runtime for .astro files.
  • Updated astro-scripts prebuild to have special bundling for client directives (tried my best in implementation 😅)

Testing

Added e2e tests

Docs

withastro/docs#3241

Also updated the types for experimental.customClientDirectives as the docs site will pull this info here. (Link to diff)

@changeset-bot
Copy link

changeset-bot bot commented May 12, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: f2644e9

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label May 12, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 12, 2023 •
edited
Loading

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

@bluwy bluwy mentioned this pull request May 12, 2023
Closed
@github-actions github-actions bot added the semver: minor Change triggers a `minor` release label May 12, 2023
@bluwy bluwy marked this pull request as ready for review May 12, 2023 16:36
@bluwy bluwy requested a review from a team as a code owner May 12, 2023 16:36
matthewp
matthewp requested changes May 12, 2023
View reviewed changes
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

Blocked because of minor release

@matthewp
Copy link
Contributor

The proposal mentions that overriding built-in directives is a non-goal. In the past some people expressed that this would be bad thing if we allowed it. I didn't notice in the code, are we doing anything that prevents someone from, for example, overriding client:idle?

@bluwy
Copy link
Member Author

bluwy commented May 13, 2023

I didn't notice in the code, are we doing anything that prevents someone from, for example, overriding client:idle?

Yeah, I added a guard here (beside the experimental check). It probably also goes in hand of whether it should use AstroError instead of Error. I thought about that, but didn't go with it as the error happens on settings/config init (maybe it still make sense to use AstroError here?)

@bluwy bluwy mentioned this pull request May 13, 2023
Merged
sarah11918
sarah11918 approved these changes May 15, 2023
View reviewed changes
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

LGTM, @bluwy! 🥳 Just left a possible link you might want to include!

packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved
@bluwy bluwy mentioned this pull request May 15, 2023
Merged
ematipico
ematipico reviewed May 16, 2023
View reviewed changes
.changeset/tall-eyes-vanish.md Show resolved Hide resolved
packages/astro/src/core/client-directive/build.ts Show resolved Hide resolved
packages/astro/test/test-utils.js Show resolved Hide resolved
scripts/cmd/prebuild.js Show resolved Hide resolved
Princesseuh
Princesseuh approved these changes May 17, 2023
View reviewed changes
Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

This looks great, awesome work @bluwy! I can't wait to see what the community do with this 😄

ematipico
ematipico approved these changes May 17, 2023
View reviewed changes
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Let's ship it! 🚢 Great work! :)

@bluwy
Copy link
Member Author

bluwy commented May 17, 2023

It's merge day. Thanks everyone who reviewed this.

@bluwy bluwy merged commit 73ec6f6 into main May 17, 2023
@bluwy bluwy deleted the plt-223-custom-client-directives branch May 17, 2023 12:51
@astrobot-houston astrobot-houston mentioned this pull request May 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sarah11918 sarah11918 sarah11918 approved these changes

@Princesseuh Princesseuh Princesseuh approved these changes

@ematipico ematipico ematipico approved these changes

@matthewp matthewp matthewp approved these changes

Assignees
No one assigned
Labels
pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bluwy @matthewp @ematipico @Princesseuh @sarah11918