feat(astro): Introduce Astro SDK - #3646

Merged
panteliselef merged 21 commits into
mainfrom
elef/introduce-astro-sdk
Jul 4, 2024
Merged

feat(astro): Introduce Astro SDK#3646
panteliselef merged 21 commits into
mainfrom
elef/introduce-astro-sdk

Conversation

@panteliselef

@panteliselefpanteliselef commented Jul 1, 2024

Copy link
Copy Markdown
Contributor

Description

This PR is tackling ECO-6 and ports the original code from astro-clerk-auth inside this monorepo as a new package called @clerk/astro.

@clerk/astro will become the official Astro SDK from Clerk.

While porting the original code the following things happened

  • Hotload is now the default method of loading clerk-js on the client.
  • Dropped the deprecated internal stores, now the stores are only available from /client/stores.
  • Dropped the /v0 module
  • Dropped the exported clerkClient as a variable, now you need to call clerkClient(context) and pass the astro context.

In follow up PRs, we will introduce a playground application for Astro, and e2e tests for the node runtime

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@panteliselefpanteliselef self-assigned this Jul 1, 2024
@changeset-bot

changeset-botBot commented Jul 1, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1d11827

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

This PR includes changesets to release 1 package
NameType
@clerk/astroPatch

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

@panteliselefpanteliselef changed the title feat(astro): Introduce Astro SDKfeat(astro): Introduce Astro SDKJul 2, 2024
@panteliselef
panteliselef marked this pull request as ready for review July 2, 2024 15:11

@wobsorianowobsoriano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@panteliselef and I reviewed this code last week, and the changes look good to me.

I've left a few suggestions for improvement.

Comment threadpackages/astro/src/async-local-storage.server.ts Outdated
Comment threadpackages/astro/src/server/get-auth.ts Outdated
Comment threadpackages/astro/src/server/clerk-middleware.ts Outdated

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A self-review would be helpful here given the size of the PR and most folks unfamiliarity with some of the more Astro-specific logic.

Comment threadpackages/astro/src/astro-components/control/Protect.astro Outdated
Comment on lines +31 to +35
const isUnauthorized =
!userId ||
(typeof Astro.props.condition === "function" &&
!Astro.props.condition(has)) ||
((Astro.props.role || Astro.props.permission) && !has(Astro.props));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: What do you think about reversing the logic here? isAuthorized feels clearer than isUnauthorized (default to unauthorized in the logic).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree, that this is hard to grok

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That's a fair point, I will added it as a todo if that's ok

import { $clerk, $csrState, $initialState } from './internal';
import { deriveState } from './utils';

export const $authStore = computed([$csrState, $initialState], (state, initialState) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this part of the public API? If so, can we add a JSDoc comment?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes it is, i will add them

Comment threadpackages/astro/src/server/build-clerk-hotload-script.ts
),
);

if (__HOTLOAD__) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

❓ why are we providing hotloading as an option?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So in the community sdk, hotloading was opt-in, now it is opt-out, but we are still allowing devs to use the astro integration with clerk-js as a direct dependency bundled with the rest of the app.

Comment on lines +27 to +30
// When the auth status is set, we trust that the middleware has already run
// Then, we don't have to re-verify the JWT here,
// we can just strip out the claims manually.
const authToken = locals.authToken || getAuthKeyFromRequest(req, 'AuthToken');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how does Astro middleware work? Does it run in the same process as the app server itself, or is it like Next where it runs somewhere else? Do we need to worry about the validity of the token here or are we safe to assume it is the token we previously verified in the middleware?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It runs on the same machine, where SSR takes place. Astro supports deployment to vercel where you can choose to use the edge middleware, but we are kind of blocking that because our middleware is using an AsyncLocalStorage

Comment threadpackages/astro/src/internal/utils/versionSelector.ts Outdated
Comment threadpackages/astro/src/integration/create-integration.ts
import { createIntegration } from './create-integration';

export default createIntegration({
mode: 'hotload',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this overridable by the user? If yes, why?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

A developer cannot manipulate this code, or call createIntegration explictly.
what the can do though is import the clerk integration from @clerk/astro/bunlded instead of @clerk/astro

The default export of @clerk/astro uses the clerk integration with hotloading.

Comment threadpackages/astro/LICENCE Outdated
Co-authored-by: Stefanos Anagnostou <anagstef@users.noreply.github.com>

@anagstefanagstef left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀 ⭐

Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/src/client/react/uiComponents.tsx
Comment threadpackages/astro/src/client/react/utils.tsx

@LekoArtsLekoArts left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First pass

Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/package.json Outdated
"astro": "^3.2.0 || ^4.0.0"
},
"engines": {
"node": ">=18.17.0"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The README says that Node 20 is required?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I had it original to node 20, but because our ci runs with v18 i had to update it. No worries i've checked the Astro minimum node version and this is compatible, also the API for AsyncLocalStorage that we are using has been stable for many versions prior to v18.

Comment on lines +31 to +35
const isUnauthorized =
!userId ||
(typeof Astro.props.condition === "function" &&
!Astro.props.condition(has)) ||
((Astro.props.role || Astro.props.permission) && !has(Astro.props));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree, that this is hard to grok

Comment on lines +14 to +16
<div id={`clerk-organization-list-${safeId}`}></div>

<script is:inline define:vars={{ props: Astro.props, safeId }}>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The safeId and setOrCreatePropMap are repeated in multiple Astro components. Can we consolidate that into shared utils?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We will resolve this in a follow up PR, i've documented it

hooks: {
'astro:config:setup': ({ config, injectScript, updateConfig, logger, command }) => {
if (config.output === 'static') {
logger.error(`${packageName} requires SSR to be turned on. Please update output to "server"`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is SSR required? Can't it also work with CSR?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Currently we support only SSR, because we are injecting the clerk-js script from the middleware. But i will add a ticket in order to explore this.

@panteliselef
panteliselef enabled auto-merge (squash) July 4, 2024 09:28
@panteliselef
panteliselef merged commit 7ae5681 into mainJul 4, 2024
@panteliselef
panteliselef deleted the elef/introduce-astro-sdk branch July 4, 2024 09:51
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@panteliselef@brkalow@wobsoriano@anagstef@LekoArts@clerk-cookie
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

feat(astro): Introduce Astro SDK - #3646

Merged
panteliselef merged 21 commits into
mainfrom
elef/introduce-astro-sdk
Jul 4, 2024
Merged

feat(astro): Introduce Astro SDK#3646
panteliselef merged 21 commits into
mainfrom
elef/introduce-astro-sdk

Conversation

@panteliselef

@panteliselefpanteliselef commented Jul 1, 2024

Copy link
Copy Markdown
Contributor

Description

This PR is tackling ECO-6 and ports the original code from astro-clerk-auth inside this monorepo as a new package called @clerk/astro.

@clerk/astro will become the official Astro SDK from Clerk.

While porting the original code the following things happened

  • Hotload is now the default method of loading clerk-js on the client.
  • Dropped the deprecated internal stores, now the stores are only available from /client/stores.
  • Dropped the /v0 module
  • Dropped the exported clerkClient as a variable, now you need to call clerkClient(context) and pass the astro context.

In follow up PRs, we will introduce a playground application for Astro, and e2e tests for the node runtime

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@panteliselefpanteliselef self-assigned this Jul 1, 2024
@changeset-bot

changeset-botBot commented Jul 1, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1d11827

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

This PR includes changesets to release 1 package
NameType
@clerk/astroPatch

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

@panteliselefpanteliselef changed the title feat(astro): Introduce Astro SDKfeat(astro): Introduce Astro SDKJul 2, 2024
@panteliselef
panteliselef marked this pull request as ready for review July 2, 2024 15:11

@wobsorianowobsoriano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@panteliselef and I reviewed this code last week, and the changes look good to me.

I've left a few suggestions for improvement.

Comment threadpackages/astro/src/async-local-storage.server.ts Outdated
Comment threadpackages/astro/src/server/get-auth.ts Outdated
Comment threadpackages/astro/src/server/clerk-middleware.ts Outdated

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A self-review would be helpful here given the size of the PR and most folks unfamiliarity with some of the more Astro-specific logic.

Comment threadpackages/astro/src/astro-components/control/Protect.astro Outdated
Comment on lines +31 to +35
const isUnauthorized =
!userId ||
(typeof Astro.props.condition === "function" &&
!Astro.props.condition(has)) ||
((Astro.props.role || Astro.props.permission) && !has(Astro.props));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: What do you think about reversing the logic here? isAuthorized feels clearer than isUnauthorized (default to unauthorized in the logic).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree, that this is hard to grok

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That's a fair point, I will added it as a todo if that's ok

import { $clerk, $csrState, $initialState } from './internal';
import { deriveState } from './utils';

export const $authStore = computed([$csrState, $initialState], (state, initialState) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this part of the public API? If so, can we add a JSDoc comment?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes it is, i will add them

Comment threadpackages/astro/src/server/build-clerk-hotload-script.ts
),
);

if (__HOTLOAD__) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

❓ why are we providing hotloading as an option?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So in the community sdk, hotloading was opt-in, now it is opt-out, but we are still allowing devs to use the astro integration with clerk-js as a direct dependency bundled with the rest of the app.

Comment on lines +27 to +30
// When the auth status is set, we trust that the middleware has already run
// Then, we don't have to re-verify the JWT here,
// we can just strip out the claims manually.
const authToken = locals.authToken || getAuthKeyFromRequest(req, 'AuthToken');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how does Astro middleware work? Does it run in the same process as the app server itself, or is it like Next where it runs somewhere else? Do we need to worry about the validity of the token here or are we safe to assume it is the token we previously verified in the middleware?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It runs on the same machine, where SSR takes place. Astro supports deployment to vercel where you can choose to use the edge middleware, but we are kind of blocking that because our middleware is using an AsyncLocalStorage

Comment threadpackages/astro/src/internal/utils/versionSelector.ts Outdated
Comment threadpackages/astro/src/integration/create-integration.ts
import { createIntegration } from './create-integration';

export default createIntegration({
mode: 'hotload',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this overridable by the user? If yes, why?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

A developer cannot manipulate this code, or call createIntegration explictly.
what the can do though is import the clerk integration from @clerk/astro/bunlded instead of @clerk/astro

The default export of @clerk/astro uses the clerk integration with hotloading.

Comment threadpackages/astro/LICENCE Outdated
Co-authored-by: Stefanos Anagnostou <anagstef@users.noreply.github.com>

@anagstefanagstef left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀 ⭐

Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/src/client/react/uiComponents.tsx
Comment threadpackages/astro/src/client/react/utils.tsx

@LekoArtsLekoArts left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First pass

Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/package.json Outdated
"astro": "^3.2.0 || ^4.0.0"
},
"engines": {
"node": ">=18.17.0"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The README says that Node 20 is required?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I had it original to node 20, but because our ci runs with v18 i had to update it. No worries i've checked the Astro minimum node version and this is compatible, also the API for AsyncLocalStorage that we are using has been stable for many versions prior to v18.

Comment on lines +31 to +35
const isUnauthorized =
!userId ||
(typeof Astro.props.condition === "function" &&
!Astro.props.condition(has)) ||
((Astro.props.role || Astro.props.permission) && !has(Astro.props));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree, that this is hard to grok

Comment on lines +14 to +16
<div id={`clerk-organization-list-${safeId}`}></div>

<script is:inline define:vars={{ props: Astro.props, safeId }}>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The safeId and setOrCreatePropMap are repeated in multiple Astro components. Can we consolidate that into shared utils?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We will resolve this in a follow up PR, i've documented it

hooks: {
'astro:config:setup': ({ config, injectScript, updateConfig, logger, command }) => {
if (config.output === 'static') {
logger.error(`${packageName} requires SSR to be turned on. Please update output to "server"`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is SSR required? Can't it also work with CSR?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Currently we support only SSR, because we are injecting the clerk-js script from the middleware. But i will add a ticket in order to explore this.

@panteliselef
panteliselef enabled auto-merge (squash) July 4, 2024 09:28
@panteliselef
panteliselef merged commit 7ae5681 into mainJul 4, 2024
@panteliselef
panteliselef deleted the elef/introduce-astro-sdk branch July 4, 2024 09:51
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@panteliselef@brkalow@wobsoriano@anagstef@LekoArts@clerk-cookie
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(astro): Introduce Astro SDK - #3646

Merged
panteliselef merged 21 commits into
mainfrom
elef/introduce-astro-sdk
Jul 4, 2024
Merged

feat(astro): Introduce Astro SDK#3646
panteliselef merged 21 commits into
mainfrom
elef/introduce-astro-sdk

Conversation

@panteliselef

@panteliselefpanteliselef commented Jul 1, 2024

Copy link
Copy Markdown
Contributor

Description

This PR is tackling ECO-6 and ports the original code from astro-clerk-auth inside this monorepo as a new package called @clerk/astro.

@clerk/astro will become the official Astro SDK from Clerk.

While porting the original code the following things happened

  • Hotload is now the default method of loading clerk-js on the client.
  • Dropped the deprecated internal stores, now the stores are only available from /client/stores.
  • Dropped the /v0 module
  • Dropped the exported clerkClient as a variable, now you need to call clerkClient(context) and pass the astro context.

In follow up PRs, we will introduce a playground application for Astro, and e2e tests for the node runtime

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@panteliselefpanteliselef self-assigned this Jul 1, 2024
@changeset-bot

changeset-botBot commented Jul 1, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1d11827

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

This PR includes changesets to release 1 package
NameType
@clerk/astroPatch

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

@panteliselefpanteliselef changed the title feat(astro): Introduce Astro SDKfeat(astro): Introduce Astro SDKJul 2, 2024
@panteliselef
panteliselef marked this pull request as ready for review July 2, 2024 15:11

@wobsorianowobsoriano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@panteliselef and I reviewed this code last week, and the changes look good to me.

I've left a few suggestions for improvement.

Comment threadpackages/astro/src/async-local-storage.server.ts Outdated
Comment threadpackages/astro/src/server/get-auth.ts Outdated
Comment threadpackages/astro/src/server/clerk-middleware.ts Outdated

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A self-review would be helpful here given the size of the PR and most folks unfamiliarity with some of the more Astro-specific logic.

Comment threadpackages/astro/src/astro-components/control/Protect.astro Outdated
Comment on lines +31 to +35
const isUnauthorized =
!userId ||
(typeof Astro.props.condition === "function" &&
!Astro.props.condition(has)) ||
((Astro.props.role || Astro.props.permission) && !has(Astro.props));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: What do you think about reversing the logic here? isAuthorized feels clearer than isUnauthorized (default to unauthorized in the logic).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree, that this is hard to grok

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That's a fair point, I will added it as a todo if that's ok

import { $clerk, $csrState, $initialState } from './internal';
import { deriveState } from './utils';

export const $authStore = computed([$csrState, $initialState], (state, initialState) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this part of the public API? If so, can we add a JSDoc comment?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes it is, i will add them

Comment threadpackages/astro/src/server/build-clerk-hotload-script.ts
),
);

if (__HOTLOAD__) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

❓ why are we providing hotloading as an option?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So in the community sdk, hotloading was opt-in, now it is opt-out, but we are still allowing devs to use the astro integration with clerk-js as a direct dependency bundled with the rest of the app.

Comment on lines +27 to +30
// When the auth status is set, we trust that the middleware has already run
// Then, we don't have to re-verify the JWT here,
// we can just strip out the claims manually.
const authToken = locals.authToken || getAuthKeyFromRequest(req, 'AuthToken');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how does Astro middleware work? Does it run in the same process as the app server itself, or is it like Next where it runs somewhere else? Do we need to worry about the validity of the token here or are we safe to assume it is the token we previously verified in the middleware?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It runs on the same machine, where SSR takes place. Astro supports deployment to vercel where you can choose to use the edge middleware, but we are kind of blocking that because our middleware is using an AsyncLocalStorage

Comment threadpackages/astro/src/internal/utils/versionSelector.ts Outdated
Comment threadpackages/astro/src/integration/create-integration.ts
import { createIntegration } from './create-integration';

export default createIntegration({
mode: 'hotload',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this overridable by the user? If yes, why?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

A developer cannot manipulate this code, or call createIntegration explictly.
what the can do though is import the clerk integration from @clerk/astro/bunlded instead of @clerk/astro

The default export of @clerk/astro uses the clerk integration with hotloading.

Comment threadpackages/astro/LICENCE Outdated
Co-authored-by: Stefanos Anagnostou <anagstef@users.noreply.github.com>

@anagstefanagstef left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀 ⭐

Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/src/client/react/uiComponents.tsx
Comment threadpackages/astro/src/client/react/utils.tsx

@LekoArtsLekoArts left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First pass

Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/package.json Outdated
"astro": "^3.2.0 || ^4.0.0"
},
"engines": {
"node": ">=18.17.0"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The README says that Node 20 is required?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I had it original to node 20, but because our ci runs with v18 i had to update it. No worries i've checked the Astro minimum node version and this is compatible, also the API for AsyncLocalStorage that we are using has been stable for many versions prior to v18.

Comment on lines +31 to +35
const isUnauthorized =
!userId ||
(typeof Astro.props.condition === "function" &&
!Astro.props.condition(has)) ||
((Astro.props.role || Astro.props.permission) && !has(Astro.props));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree, that this is hard to grok

Comment on lines +14 to +16
<div id={`clerk-organization-list-${safeId}`}></div>

<script is:inline define:vars={{ props: Astro.props, safeId }}>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The safeId and setOrCreatePropMap are repeated in multiple Astro components. Can we consolidate that into shared utils?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We will resolve this in a follow up PR, i've documented it

hooks: {
'astro:config:setup': ({ config, injectScript, updateConfig, logger, command }) => {
if (config.output === 'static') {
logger.error(`${packageName} requires SSR to be turned on. Please update output to "server"`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is SSR required? Can't it also work with CSR?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Currently we support only SSR, because we are injecting the clerk-js script from the middleware. But i will add a ticket in order to explore this.

@panteliselef
panteliselef enabled auto-merge (squash) July 4, 2024 09:28
@panteliselef
panteliselef merged commit 7ae5681 into mainJul 4, 2024
@panteliselef
panteliselef deleted the elef/introduce-astro-sdk branch July 4, 2024 09:51
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@panteliselef@brkalow@wobsoriano@anagstef@LekoArts@clerk-cookie
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(astro): Introduce Astro SDK - #3646

Merged
panteliselef merged 21 commits into
mainfrom
elef/introduce-astro-sdk
Jul 4, 2024
Merged

feat(astro): Introduce Astro SDK#3646
panteliselef merged 21 commits into
mainfrom
elef/introduce-astro-sdk

Conversation

@panteliselef

@panteliselefpanteliselef commented Jul 1, 2024

Copy link
Copy Markdown
Contributor

Description

This PR is tackling ECO-6 and ports the original code from astro-clerk-auth inside this monorepo as a new package called @clerk/astro.

@clerk/astro will become the official Astro SDK from Clerk.

While porting the original code the following things happened

  • Hotload is now the default method of loading clerk-js on the client.
  • Dropped the deprecated internal stores, now the stores are only available from /client/stores.
  • Dropped the /v0 module
  • Dropped the exported clerkClient as a variable, now you need to call clerkClient(context) and pass the astro context.

In follow up PRs, we will introduce a playground application for Astro, and e2e tests for the node runtime

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@panteliselefpanteliselef self-assigned this Jul 1, 2024
@changeset-bot

changeset-botBot commented Jul 1, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1d11827

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

This PR includes changesets to release 1 package
NameType
@clerk/astroPatch

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

@panteliselefpanteliselef changed the title feat(astro): Introduce Astro SDKfeat(astro): Introduce Astro SDKJul 2, 2024
@panteliselef
panteliselef marked this pull request as ready for review July 2, 2024 15:11

@wobsorianowobsoriano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@panteliselef and I reviewed this code last week, and the changes look good to me.

I've left a few suggestions for improvement.

Comment threadpackages/astro/src/async-local-storage.server.ts Outdated
Comment threadpackages/astro/src/server/get-auth.ts Outdated
Comment threadpackages/astro/src/server/clerk-middleware.ts Outdated

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A self-review would be helpful here given the size of the PR and most folks unfamiliarity with some of the more Astro-specific logic.

Comment threadpackages/astro/src/astro-components/control/Protect.astro Outdated
Comment on lines +31 to +35
const isUnauthorized =
!userId ||
(typeof Astro.props.condition === "function" &&
!Astro.props.condition(has)) ||
((Astro.props.role || Astro.props.permission) && !has(Astro.props));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: What do you think about reversing the logic here? isAuthorized feels clearer than isUnauthorized (default to unauthorized in the logic).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree, that this is hard to grok

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That's a fair point, I will added it as a todo if that's ok

import { $clerk, $csrState, $initialState } from './internal';
import { deriveState } from './utils';

export const $authStore = computed([$csrState, $initialState], (state, initialState) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this part of the public API? If so, can we add a JSDoc comment?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes it is, i will add them

Comment threadpackages/astro/src/server/build-clerk-hotload-script.ts
),
);

if (__HOTLOAD__) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

❓ why are we providing hotloading as an option?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So in the community sdk, hotloading was opt-in, now it is opt-out, but we are still allowing devs to use the astro integration with clerk-js as a direct dependency bundled with the rest of the app.

Comment on lines +27 to +30
// When the auth status is set, we trust that the middleware has already run
// Then, we don't have to re-verify the JWT here,
// we can just strip out the claims manually.
const authToken = locals.authToken || getAuthKeyFromRequest(req, 'AuthToken');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how does Astro middleware work? Does it run in the same process as the app server itself, or is it like Next where it runs somewhere else? Do we need to worry about the validity of the token here or are we safe to assume it is the token we previously verified in the middleware?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It runs on the same machine, where SSR takes place. Astro supports deployment to vercel where you can choose to use the edge middleware, but we are kind of blocking that because our middleware is using an AsyncLocalStorage

Comment threadpackages/astro/src/internal/utils/versionSelector.ts Outdated
Comment threadpackages/astro/src/integration/create-integration.ts
import { createIntegration } from './create-integration';

export default createIntegration({
mode: 'hotload',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this overridable by the user? If yes, why?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

A developer cannot manipulate this code, or call createIntegration explictly.
what the can do though is import the clerk integration from @clerk/astro/bunlded instead of @clerk/astro

The default export of @clerk/astro uses the clerk integration with hotloading.

Comment threadpackages/astro/LICENCE Outdated
Co-authored-by: Stefanos Anagnostou <anagstef@users.noreply.github.com>

@anagstefanagstef left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀 ⭐

Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/src/client/react/uiComponents.tsx
Comment threadpackages/astro/src/client/react/utils.tsx

@LekoArtsLekoArts left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First pass

Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/package.json Outdated
"astro": "^3.2.0 || ^4.0.0"
},
"engines": {
"node": ">=18.17.0"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The README says that Node 20 is required?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I had it original to node 20, but because our ci runs with v18 i had to update it. No worries i've checked the Astro minimum node version and this is compatible, also the API for AsyncLocalStorage that we are using has been stable for many versions prior to v18.

Comment on lines +31 to +35
const isUnauthorized =
!userId ||
(typeof Astro.props.condition === "function" &&
!Astro.props.condition(has)) ||
((Astro.props.role || Astro.props.permission) && !has(Astro.props));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree, that this is hard to grok

Comment on lines +14 to +16
<div id={`clerk-organization-list-${safeId}`}></div>

<script is:inline define:vars={{ props: Astro.props, safeId }}>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The safeId and setOrCreatePropMap are repeated in multiple Astro components. Can we consolidate that into shared utils?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We will resolve this in a follow up PR, i've documented it

hooks: {
'astro:config:setup': ({ config, injectScript, updateConfig, logger, command }) => {
if (config.output === 'static') {
logger.error(`${packageName} requires SSR to be turned on. Please update output to "server"`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is SSR required? Can't it also work with CSR?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Currently we support only SSR, because we are injecting the clerk-js script from the middleware. But i will add a ticket in order to explore this.

@panteliselef
panteliselef enabled auto-merge (squash) July 4, 2024 09:28
@panteliselef
panteliselef merged commit 7ae5681 into mainJul 4, 2024
@panteliselef
panteliselef deleted the elef/introduce-astro-sdk branch July 4, 2024 09:51
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@panteliselef@brkalow@wobsoriano@anagstef@LekoArts@clerk-cookie
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

feat(astro): Introduce Astro SDK - #3646

Merged
panteliselef merged 21 commits into
mainfrom
elef/introduce-astro-sdk
Jul 4, 2024
Merged

feat(astro): Introduce Astro SDK#3646
panteliselef merged 21 commits into
mainfrom
elef/introduce-astro-sdk

Conversation

@panteliselef

@panteliselefpanteliselef commented Jul 1, 2024

Copy link
Copy Markdown
Contributor

Description

This PR is tackling ECO-6 and ports the original code from astro-clerk-auth inside this monorepo as a new package called @clerk/astro.

@clerk/astro will become the official Astro SDK from Clerk.

While porting the original code the following things happened

  • Hotload is now the default method of loading clerk-js on the client.
  • Dropped the deprecated internal stores, now the stores are only available from /client/stores.
  • Dropped the /v0 module
  • Dropped the exported clerkClient as a variable, now you need to call clerkClient(context) and pass the astro context.

In follow up PRs, we will introduce a playground application for Astro, and e2e tests for the node runtime

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@panteliselefpanteliselef self-assigned this Jul 1, 2024
@changeset-bot

changeset-botBot commented Jul 1, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1d11827

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

This PR includes changesets to release 1 package
NameType
@clerk/astroPatch

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

@panteliselefpanteliselef changed the title feat(astro): Introduce Astro SDKfeat(astro): Introduce Astro SDKJul 2, 2024
@panteliselef
panteliselef marked this pull request as ready for review July 2, 2024 15:11

@wobsorianowobsoriano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@panteliselef and I reviewed this code last week, and the changes look good to me.

I've left a few suggestions for improvement.

Comment threadpackages/astro/src/async-local-storage.server.ts Outdated
Comment threadpackages/astro/src/server/get-auth.ts Outdated
Comment threadpackages/astro/src/server/clerk-middleware.ts Outdated

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A self-review would be helpful here given the size of the PR and most folks unfamiliarity with some of the more Astro-specific logic.

Comment threadpackages/astro/src/astro-components/control/Protect.astro Outdated
Comment on lines +31 to +35
const isUnauthorized =
!userId ||
(typeof Astro.props.condition === "function" &&
!Astro.props.condition(has)) ||
((Astro.props.role || Astro.props.permission) && !has(Astro.props));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: What do you think about reversing the logic here? isAuthorized feels clearer than isUnauthorized (default to unauthorized in the logic).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree, that this is hard to grok

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That's a fair point, I will added it as a todo if that's ok

import { $clerk, $csrState, $initialState } from './internal';
import { deriveState } from './utils';

export const $authStore = computed([$csrState, $initialState], (state, initialState) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this part of the public API? If so, can we add a JSDoc comment?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes it is, i will add them

Comment threadpackages/astro/src/server/build-clerk-hotload-script.ts
),
);

if (__HOTLOAD__) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

❓ why are we providing hotloading as an option?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So in the community sdk, hotloading was opt-in, now it is opt-out, but we are still allowing devs to use the astro integration with clerk-js as a direct dependency bundled with the rest of the app.

Comment on lines +27 to +30
// When the auth status is set, we trust that the middleware has already run
// Then, we don't have to re-verify the JWT here,
// we can just strip out the claims manually.
const authToken = locals.authToken || getAuthKeyFromRequest(req, 'AuthToken');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how does Astro middleware work? Does it run in the same process as the app server itself, or is it like Next where it runs somewhere else? Do we need to worry about the validity of the token here or are we safe to assume it is the token we previously verified in the middleware?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It runs on the same machine, where SSR takes place. Astro supports deployment to vercel where you can choose to use the edge middleware, but we are kind of blocking that because our middleware is using an AsyncLocalStorage

Comment threadpackages/astro/src/internal/utils/versionSelector.ts Outdated
Comment threadpackages/astro/src/integration/create-integration.ts
import { createIntegration } from './create-integration';

export default createIntegration({
mode: 'hotload',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this overridable by the user? If yes, why?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

A developer cannot manipulate this code, or call createIntegration explictly.
what the can do though is import the clerk integration from @clerk/astro/bunlded instead of @clerk/astro

The default export of @clerk/astro uses the clerk integration with hotloading.

Comment threadpackages/astro/LICENCE Outdated
Co-authored-by: Stefanos Anagnostou <anagstef@users.noreply.github.com>

@anagstefanagstef left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀 ⭐

Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/src/client/react/uiComponents.tsx
Comment threadpackages/astro/src/client/react/utils.tsx

@LekoArtsLekoArts left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First pass

Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/package.json Outdated
"astro": "^3.2.0 || ^4.0.0"
},
"engines": {
"node": ">=18.17.0"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The README says that Node 20 is required?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I had it original to node 20, but because our ci runs with v18 i had to update it. No worries i've checked the Astro minimum node version and this is compatible, also the API for AsyncLocalStorage that we are using has been stable for many versions prior to v18.

Comment on lines +31 to +35
const isUnauthorized =
!userId ||
(typeof Astro.props.condition === "function" &&
!Astro.props.condition(has)) ||
((Astro.props.role || Astro.props.permission) && !has(Astro.props));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree, that this is hard to grok

Comment on lines +14 to +16
<div id={`clerk-organization-list-${safeId}`}></div>

<script is:inline define:vars={{ props: Astro.props, safeId }}>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The safeId and setOrCreatePropMap are repeated in multiple Astro components. Can we consolidate that into shared utils?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We will resolve this in a follow up PR, i've documented it

hooks: {
'astro:config:setup': ({ config, injectScript, updateConfig, logger, command }) => {
if (config.output === 'static') {
logger.error(`${packageName} requires SSR to be turned on. Please update output to "server"`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is SSR required? Can't it also work with CSR?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Currently we support only SSR, because we are injecting the clerk-js script from the middleware. But i will add a ticket in order to explore this.

@panteliselef
panteliselef enabled auto-merge (squash) July 4, 2024 09:28
@panteliselef
panteliselef merged commit 7ae5681 into mainJul 4, 2024
@panteliselef
panteliselef deleted the elef/introduce-astro-sdk branch July 4, 2024 09:51
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@panteliselef@brkalow@wobsoriano@anagstef@LekoArts@clerk-cookie
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(astro): Introduce Astro SDK - #3646

Merged
panteliselef merged 21 commits into
mainfrom
elef/introduce-astro-sdk
Jul 4, 2024
Merged

feat(astro): Introduce Astro SDK#3646
panteliselef merged 21 commits into
mainfrom
elef/introduce-astro-sdk

Conversation

@panteliselef

@panteliselefpanteliselef commented Jul 1, 2024

Copy link
Copy Markdown
Contributor

Description

This PR is tackling ECO-6 and ports the original code from astro-clerk-auth inside this monorepo as a new package called @clerk/astro.

@clerk/astro will become the official Astro SDK from Clerk.

While porting the original code the following things happened

  • Hotload is now the default method of loading clerk-js on the client.
  • Dropped the deprecated internal stores, now the stores are only available from /client/stores.
  • Dropped the /v0 module
  • Dropped the exported clerkClient as a variable, now you need to call clerkClient(context) and pass the astro context.

In follow up PRs, we will introduce a playground application for Astro, and e2e tests for the node runtime

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@panteliselefpanteliselef self-assigned this Jul 1, 2024
@changeset-bot

changeset-botBot commented Jul 1, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1d11827

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

This PR includes changesets to release 1 package
NameType
@clerk/astroPatch

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

@panteliselefpanteliselef changed the title feat(astro): Introduce Astro SDKfeat(astro): Introduce Astro SDKJul 2, 2024
@panteliselef
panteliselef marked this pull request as ready for review July 2, 2024 15:11

@wobsorianowobsoriano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@panteliselef and I reviewed this code last week, and the changes look good to me.

I've left a few suggestions for improvement.

Comment threadpackages/astro/src/async-local-storage.server.ts Outdated
Comment threadpackages/astro/src/server/get-auth.ts Outdated
Comment threadpackages/astro/src/server/clerk-middleware.ts Outdated

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A self-review would be helpful here given the size of the PR and most folks unfamiliarity with some of the more Astro-specific logic.

Comment threadpackages/astro/src/astro-components/control/Protect.astro Outdated
Comment on lines +31 to +35
const isUnauthorized =
!userId ||
(typeof Astro.props.condition === "function" &&
!Astro.props.condition(has)) ||
((Astro.props.role || Astro.props.permission) && !has(Astro.props));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: What do you think about reversing the logic here? isAuthorized feels clearer than isUnauthorized (default to unauthorized in the logic).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree, that this is hard to grok

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That's a fair point, I will added it as a todo if that's ok

import { $clerk, $csrState, $initialState } from './internal';
import { deriveState } from './utils';

export const $authStore = computed([$csrState, $initialState], (state, initialState) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this part of the public API? If so, can we add a JSDoc comment?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes it is, i will add them

Comment threadpackages/astro/src/server/build-clerk-hotload-script.ts
),
);

if (__HOTLOAD__) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

❓ why are we providing hotloading as an option?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So in the community sdk, hotloading was opt-in, now it is opt-out, but we are still allowing devs to use the astro integration with clerk-js as a direct dependency bundled with the rest of the app.

Comment on lines +27 to +30
// When the auth status is set, we trust that the middleware has already run
// Then, we don't have to re-verify the JWT here,
// we can just strip out the claims manually.
const authToken = locals.authToken || getAuthKeyFromRequest(req, 'AuthToken');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how does Astro middleware work? Does it run in the same process as the app server itself, or is it like Next where it runs somewhere else? Do we need to worry about the validity of the token here or are we safe to assume it is the token we previously verified in the middleware?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It runs on the same machine, where SSR takes place. Astro supports deployment to vercel where you can choose to use the edge middleware, but we are kind of blocking that because our middleware is using an AsyncLocalStorage

Comment threadpackages/astro/src/internal/utils/versionSelector.ts Outdated
Comment threadpackages/astro/src/integration/create-integration.ts
import { createIntegration } from './create-integration';

export default createIntegration({
mode: 'hotload',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this overridable by the user? If yes, why?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

A developer cannot manipulate this code, or call createIntegration explictly.
what the can do though is import the clerk integration from @clerk/astro/bunlded instead of @clerk/astro

The default export of @clerk/astro uses the clerk integration with hotloading.

Comment threadpackages/astro/LICENCE Outdated
Co-authored-by: Stefanos Anagnostou <anagstef@users.noreply.github.com>

@anagstefanagstef left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀 ⭐

Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/src/client/react/uiComponents.tsx
Comment threadpackages/astro/src/client/react/utils.tsx

@LekoArtsLekoArts left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First pass

Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/package.json Outdated
"astro": "^3.2.0 || ^4.0.0"
},
"engines": {
"node": ">=18.17.0"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The README says that Node 20 is required?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I had it original to node 20, but because our ci runs with v18 i had to update it. No worries i've checked the Astro minimum node version and this is compatible, also the API for AsyncLocalStorage that we are using has been stable for many versions prior to v18.

Comment on lines +31 to +35
const isUnauthorized =
!userId ||
(typeof Astro.props.condition === "function" &&
!Astro.props.condition(has)) ||
((Astro.props.role || Astro.props.permission) && !has(Astro.props));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree, that this is hard to grok

Comment on lines +14 to +16
<div id={`clerk-organization-list-${safeId}`}></div>

<script is:inline define:vars={{ props: Astro.props, safeId }}>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The safeId and setOrCreatePropMap are repeated in multiple Astro components. Can we consolidate that into shared utils?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We will resolve this in a follow up PR, i've documented it

hooks: {
'astro:config:setup': ({ config, injectScript, updateConfig, logger, command }) => {
if (config.output === 'static') {
logger.error(`${packageName} requires SSR to be turned on. Please update output to "server"`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is SSR required? Can't it also work with CSR?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Currently we support only SSR, because we are injecting the clerk-js script from the middleware. But i will add a ticket in order to explore this.

@panteliselef
panteliselef enabled auto-merge (squash) July 4, 2024 09:28
@panteliselef
panteliselef merged commit 7ae5681 into mainJul 4, 2024
@panteliselef
panteliselef deleted the elef/introduce-astro-sdk branch July 4, 2024 09:51
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@panteliselef@brkalow@wobsoriano@anagstef@LekoArts@clerk-cookie
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(astro): Introduce Astro SDK - #3646

Merged
panteliselef merged 21 commits into
mainfrom
elef/introduce-astro-sdk
Jul 4, 2024
Merged

feat(astro): Introduce Astro SDK#3646
panteliselef merged 21 commits into
mainfrom
elef/introduce-astro-sdk

Conversation

@panteliselef

@panteliselefpanteliselef commented Jul 1, 2024

Copy link
Copy Markdown
Contributor

Description

This PR is tackling ECO-6 and ports the original code from astro-clerk-auth inside this monorepo as a new package called @clerk/astro.

@clerk/astro will become the official Astro SDK from Clerk.

While porting the original code the following things happened

  • Hotload is now the default method of loading clerk-js on the client.
  • Dropped the deprecated internal stores, now the stores are only available from /client/stores.
  • Dropped the /v0 module
  • Dropped the exported clerkClient as a variable, now you need to call clerkClient(context) and pass the astro context.

In follow up PRs, we will introduce a playground application for Astro, and e2e tests for the node runtime

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@panteliselefpanteliselef self-assigned this Jul 1, 2024
@changeset-bot

changeset-botBot commented Jul 1, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1d11827

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

This PR includes changesets to release 1 package
NameType
@clerk/astroPatch

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

@panteliselefpanteliselef changed the title feat(astro): Introduce Astro SDKfeat(astro): Introduce Astro SDKJul 2, 2024
@panteliselef
panteliselef marked this pull request as ready for review July 2, 2024 15:11

@wobsorianowobsoriano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@panteliselef and I reviewed this code last week, and the changes look good to me.

I've left a few suggestions for improvement.

Comment threadpackages/astro/src/async-local-storage.server.ts Outdated
Comment threadpackages/astro/src/server/get-auth.ts Outdated
Comment threadpackages/astro/src/server/clerk-middleware.ts Outdated

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A self-review would be helpful here given the size of the PR and most folks unfamiliarity with some of the more Astro-specific logic.

Comment threadpackages/astro/src/astro-components/control/Protect.astro Outdated
Comment on lines +31 to +35
const isUnauthorized =
!userId ||
(typeof Astro.props.condition === "function" &&
!Astro.props.condition(has)) ||
((Astro.props.role || Astro.props.permission) && !has(Astro.props));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: What do you think about reversing the logic here? isAuthorized feels clearer than isUnauthorized (default to unauthorized in the logic).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree, that this is hard to grok

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That's a fair point, I will added it as a todo if that's ok

import { $clerk, $csrState, $initialState } from './internal';
import { deriveState } from './utils';

export const $authStore = computed([$csrState, $initialState], (state, initialState) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this part of the public API? If so, can we add a JSDoc comment?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes it is, i will add them

Comment threadpackages/astro/src/server/build-clerk-hotload-script.ts
),
);

if (__HOTLOAD__) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

❓ why are we providing hotloading as an option?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So in the community sdk, hotloading was opt-in, now it is opt-out, but we are still allowing devs to use the astro integration with clerk-js as a direct dependency bundled with the rest of the app.

Comment on lines +27 to +30
// When the auth status is set, we trust that the middleware has already run
// Then, we don't have to re-verify the JWT here,
// we can just strip out the claims manually.
const authToken = locals.authToken || getAuthKeyFromRequest(req, 'AuthToken');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how does Astro middleware work? Does it run in the same process as the app server itself, or is it like Next where it runs somewhere else? Do we need to worry about the validity of the token here or are we safe to assume it is the token we previously verified in the middleware?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It runs on the same machine, where SSR takes place. Astro supports deployment to vercel where you can choose to use the edge middleware, but we are kind of blocking that because our middleware is using an AsyncLocalStorage

Comment threadpackages/astro/src/internal/utils/versionSelector.ts Outdated
Comment threadpackages/astro/src/integration/create-integration.ts
import { createIntegration } from './create-integration';

export default createIntegration({
mode: 'hotload',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this overridable by the user? If yes, why?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

A developer cannot manipulate this code, or call createIntegration explictly.
what the can do though is import the clerk integration from @clerk/astro/bunlded instead of @clerk/astro

The default export of @clerk/astro uses the clerk integration with hotloading.

Comment threadpackages/astro/LICENCE Outdated
Co-authored-by: Stefanos Anagnostou <anagstef@users.noreply.github.com>

@anagstefanagstef left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀 ⭐

Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/src/client/react/uiComponents.tsx
Comment threadpackages/astro/src/client/react/utils.tsx

@LekoArtsLekoArts left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First pass

Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/package.json Outdated
"astro": "^3.2.0 || ^4.0.0"
},
"engines": {
"node": ">=18.17.0"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The README says that Node 20 is required?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I had it original to node 20, but because our ci runs with v18 i had to update it. No worries i've checked the Astro minimum node version and this is compatible, also the API for AsyncLocalStorage that we are using has been stable for many versions prior to v18.

Comment on lines +31 to +35
const isUnauthorized =
!userId ||
(typeof Astro.props.condition === "function" &&
!Astro.props.condition(has)) ||
((Astro.props.role || Astro.props.permission) && !has(Astro.props));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree, that this is hard to grok

Comment on lines +14 to +16
<div id={`clerk-organization-list-${safeId}`}></div>

<script is:inline define:vars={{ props: Astro.props, safeId }}>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The safeId and setOrCreatePropMap are repeated in multiple Astro components. Can we consolidate that into shared utils?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We will resolve this in a follow up PR, i've documented it

hooks: {
'astro:config:setup': ({ config, injectScript, updateConfig, logger, command }) => {
if (config.output === 'static') {
logger.error(`${packageName} requires SSR to be turned on. Please update output to "server"`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is SSR required? Can't it also work with CSR?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Currently we support only SSR, because we are injecting the clerk-js script from the middleware. But i will add a ticket in order to explore this.

@panteliselef
panteliselef enabled auto-merge (squash) July 4, 2024 09:28
@panteliselef
panteliselef merged commit 7ae5681 into mainJul 4, 2024
@panteliselef
panteliselef deleted the elef/introduce-astro-sdk branch July 4, 2024 09:51
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@panteliselef@brkalow@wobsoriano@anagstef@LekoArts@clerk-cookie
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

feat(astro): Introduce Astro SDK - #3646

Merged
panteliselef merged 21 commits into
mainfrom
elef/introduce-astro-sdk
Jul 4, 2024
Merged

feat(astro): Introduce Astro SDK#3646
panteliselef merged 21 commits into
mainfrom
elef/introduce-astro-sdk

Conversation

@panteliselef

@panteliselefpanteliselef commented Jul 1, 2024

Copy link
Copy Markdown
Contributor

Description

This PR is tackling ECO-6 and ports the original code from astro-clerk-auth inside this monorepo as a new package called @clerk/astro.

@clerk/astro will become the official Astro SDK from Clerk.

While porting the original code the following things happened

  • Hotload is now the default method of loading clerk-js on the client.
  • Dropped the deprecated internal stores, now the stores are only available from /client/stores.
  • Dropped the /v0 module
  • Dropped the exported clerkClient as a variable, now you need to call clerkClient(context) and pass the astro context.

In follow up PRs, we will introduce a playground application for Astro, and e2e tests for the node runtime

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@panteliselefpanteliselef self-assigned this Jul 1, 2024
@changeset-bot

changeset-botBot commented Jul 1, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1d11827

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

This PR includes changesets to release 1 package
NameType
@clerk/astroPatch

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

@panteliselefpanteliselef changed the title feat(astro): Introduce Astro SDKfeat(astro): Introduce Astro SDKJul 2, 2024
@panteliselef
panteliselef marked this pull request as ready for review July 2, 2024 15:11

@wobsorianowobsoriano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@panteliselef and I reviewed this code last week, and the changes look good to me.

I've left a few suggestions for improvement.

Comment threadpackages/astro/src/async-local-storage.server.ts Outdated
Comment threadpackages/astro/src/server/get-auth.ts Outdated
Comment threadpackages/astro/src/server/clerk-middleware.ts Outdated

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A self-review would be helpful here given the size of the PR and most folks unfamiliarity with some of the more Astro-specific logic.

Comment threadpackages/astro/src/astro-components/control/Protect.astro Outdated
Comment on lines +31 to +35
const isUnauthorized =
!userId ||
(typeof Astro.props.condition === "function" &&
!Astro.props.condition(has)) ||
((Astro.props.role || Astro.props.permission) && !has(Astro.props));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: What do you think about reversing the logic here? isAuthorized feels clearer than isUnauthorized (default to unauthorized in the logic).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree, that this is hard to grok

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That's a fair point, I will added it as a todo if that's ok

import { $clerk, $csrState, $initialState } from './internal';
import { deriveState } from './utils';

export const $authStore = computed([$csrState, $initialState], (state, initialState) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this part of the public API? If so, can we add a JSDoc comment?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes it is, i will add them

Comment threadpackages/astro/src/server/build-clerk-hotload-script.ts
),
);

if (__HOTLOAD__) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

❓ why are we providing hotloading as an option?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So in the community sdk, hotloading was opt-in, now it is opt-out, but we are still allowing devs to use the astro integration with clerk-js as a direct dependency bundled with the rest of the app.

Comment on lines +27 to +30
// When the auth status is set, we trust that the middleware has already run
// Then, we don't have to re-verify the JWT here,
// we can just strip out the claims manually.
const authToken = locals.authToken || getAuthKeyFromRequest(req, 'AuthToken');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how does Astro middleware work? Does it run in the same process as the app server itself, or is it like Next where it runs somewhere else? Do we need to worry about the validity of the token here or are we safe to assume it is the token we previously verified in the middleware?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It runs on the same machine, where SSR takes place. Astro supports deployment to vercel where you can choose to use the edge middleware, but we are kind of blocking that because our middleware is using an AsyncLocalStorage

Comment threadpackages/astro/src/internal/utils/versionSelector.ts Outdated
Comment threadpackages/astro/src/integration/create-integration.ts
import { createIntegration } from './create-integration';

export default createIntegration({
mode: 'hotload',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this overridable by the user? If yes, why?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

A developer cannot manipulate this code, or call createIntegration explictly.
what the can do though is import the clerk integration from @clerk/astro/bunlded instead of @clerk/astro

The default export of @clerk/astro uses the clerk integration with hotloading.

Comment threadpackages/astro/LICENCE Outdated
Co-authored-by: Stefanos Anagnostou <anagstef@users.noreply.github.com>

@anagstefanagstef left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀 ⭐

Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/src/client/react/uiComponents.tsx
Comment threadpackages/astro/src/client/react/utils.tsx

@LekoArtsLekoArts left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First pass

Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/README.md Outdated
Comment threadpackages/astro/package.json Outdated
"astro": "^3.2.0 || ^4.0.0"
},
"engines": {
"node": ">=18.17.0"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The README says that Node 20 is required?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I had it original to node 20, but because our ci runs with v18 i had to update it. No worries i've checked the Astro minimum node version and this is compatible, also the API for AsyncLocalStorage that we are using has been stable for many versions prior to v18.

Comment on lines +31 to +35
const isUnauthorized =
!userId ||
(typeof Astro.props.condition === "function" &&
!Astro.props.condition(has)) ||
((Astro.props.role || Astro.props.permission) && !has(Astro.props));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree, that this is hard to grok

Comment on lines +14 to +16
<div id={`clerk-organization-list-${safeId}`}></div>

<script is:inline define:vars={{ props: Astro.props, safeId }}>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The safeId and setOrCreatePropMap are repeated in multiple Astro components. Can we consolidate that into shared utils?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We will resolve this in a follow up PR, i've documented it

hooks: {
'astro:config:setup': ({ config, injectScript, updateConfig, logger, command }) => {
if (config.output === 'static') {
logger.error(`${packageName} requires SSR to be turned on. Please update output to "server"`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is SSR required? Can't it also work with CSR?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Currently we support only SSR, because we are injecting the clerk-js script from the middleware. But i will add a ticket in order to explore this.

@panteliselef
panteliselef enabled auto-merge (squash) July 4, 2024 09:28
@panteliselef
panteliselef merged commit 7ae5681 into mainJul 4, 2024
@panteliselef
panteliselef deleted the elef/introduce-astro-sdk branch July 4, 2024 09:51
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@panteliselef@brkalow@wobsoriano@anagstef@LekoArts@clerk-cookie