-
Notifications
You must be signed in to change notification settings - Fork 299
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
feat(astro): Add CSR support #3911
feat(astro): Add CSR support #3911
Conversation
🦋 Changeset detectedLatest commit: aadf639 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
9daf40b
to
c892565
Compare
cf4f5c6
to
ecaac43
Compare
8bfae44
to
39df490
Compare
1b466bd
to
3fec45b
Compare
chore: Remove bundled file chore(astro): Add back missing declarations
!snapshot |
Hey @wobsoriano - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact |
* | ||
* $isLoadedStore.subscribe((authloaded => console.log(loaded)) | ||
*/ | ||
export const $isLoadedStore = computed([$csrState], state => state.isLoaded); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reliability issue with $clerkStore
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be added as JSdoc for other maintainers
|
||
<style> | ||
clerk-signed-out[hidden] { | ||
display: none !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case someone would like to animate in/out the children of <SignedOut/>
would this be possible ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the control components to have class
props so they can override it. We'll mention this in the CSR reference docs when added 👍🏼
If only there's a way to not have a wrapper element and dynamically render the contents, but sadly no because of how astro components works
permission?: never; | ||
}; | ||
import { isStaticOutput } from "virtual:@clerk/astro/config"; | ||
import type { ProtectProps } from '../../types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work because we now expose @clerk/astro/types
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are only copying the types.ts
file to so that it can still be resolvable when published. We don't export the types in package.json
still, though. I don't see any helpful type to export 🤔
integration/presets/astro.ts
Outdated
@@ -20,6 +20,9 @@ const astroNode = applicationConfig() | |||
.addDependency('@clerk/types', clerkTypesLocal) | |||
.addDependency('@clerk/localizations', clerkLocalizationLocal); | |||
|
|||
const astroStatic = astroNode.clone().setName('astro-static').useTemplate(templates['astro-static']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add an e2e template with hybrid and prerender = true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good to have but I think if we do hybrid test we can have both prerendered pages and just remove static e2e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed static test in favor of hybrid output test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! 🔥
Co-authored-by: Lennart <[email protected]>
Description
This PR adds support for
static
andhybrid
outputs in Astro by making our control components isomorphic. A basic e2e test has been added as well.Limitations:
hybrid
output, aisStatic
prop is available to control which component to use. It's going to use CSR components by default (since pages are prerendered by default in hybrid output).Have to separate the web components to their own files instead of putting them inside theFixed here*CSR.astro
components because of some Vite issues. Please see ECO-17 for more info.Closes ECO-17
Checklist
npm test
runs as expected.npm run build
runs as expected.Type of change