-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Reworks RSC server entries and route manifest building to derive from routes and include if route info related to authentication #10572
Conversation
@@ -49,7 +49,7 @@ export class RWRoute extends BaseNode { | |||
?.getOpeningElement() | |||
?.getTagNameNode() | |||
?.getText() | |||
return tagText === 'Private' | |||
return tagText === 'Private' || tagText === 'PrivateSet' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to keep "Private"? I think it has to be "PrivateSet" now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say lets remove Private
in a separate PR, and remove it from exports too!
packages/vite/src/lib/entries.ts
Outdated
for (const page of pages) { | ||
entries[page.importName] = page.path | ||
entries[page.const_] = ensurePosixPath(page.path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure of the use of const_
is appropriate, but it generates the correct key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do again? Sorry can't remember 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use on const_
here provides the expected key for the entries file that is compatible with the prior way of serving the key:
"UserExampleNewUserExamplePage": "assets/UserExampleNewUserExamplePage-CPG6dVwh.mjs",
"UserExampleEditUserExamplePage": "assets/UserExampleEditUserExamplePage-BfvIJHxv.mjs",
UserExampleNewUserExamplePage
is the key.
Before, the processPagesDir
created a new type that enriches the page with and importName
based on the glob file name:
export const processPagesDir = (
webPagesDir: string = getPaths().web.pages,
): Array<PagesDependency> => {
const pagePaths = fg.sync('**/*Page.{js,jsx,ts,tsx}', {
cwd: webPagesDir,
ignore: ['node_modules'],
})
return pagePaths.map((pagePath) => {
const p = path.parse(pagePath)
const importName = p.dir.replace(/\//g, '') <<<<<----
const importPath = importStatementPath(
path.join(webPagesDir, p.dir, p.name),
Whatever const_
is it appears to have the same info we need without having to process the paths.
I think it comes from structure model File or BaseNode.
…t that can determine private routes.
Nice one @dthyresson! I think we'll need to go one step further. Right now in this PR it just tells us whether its private or not: "/about": {
"name": "about",
"bundle": null,
"matchRegexString": "^/about$",
"pathDefinition": "/about",
"hasParams": false,
"routeHooks": null,
"redirect": null,
"relativeFilePath": "pages/AboutPage/AboutPage.tsx",
+ "isPrivate": true
}, But I think the missing steps are:
|
@dac I added a <Route path="/" page={HomePage} name="home" />
<PrivateSet unauthenticated="home">
<Route path="/about" page={AboutPage} name="about" />
<Route path="/about-secret" page={AboutPage} name="aboutSecret" />
</PrivateSet>
<Route path="/multi-cell" page={MultiCellPage} name="multiCell" /> now results in: {
"/": {
"name": "home",
"bundle": null,
"matchRegexString": "^/$",
"pathDefinition": "/",
"hasParams": false,
"routeHooks": null,
"redirect": null,
"relativeFilePath": "pages/HomePage/HomePage.tsx",
"isPrivate": false
},
"/about": {
"name": "about",
"bundle": null,
"matchRegexString": "^/about$",
"pathDefinition": "/about",
"hasParams": false,
"routeHooks": null,
"redirect": null,
"relativeFilePath": "pages/AboutPage/AboutPage.tsx",
"isPrivate": true,
"unauthenticated": "home"
},
"/about-secret": {
"name": "aboutSecret",
"bundle": null,
"matchRegexString": "^/about-secret$",
"pathDefinition": "/about-secret",
"hasParams": false,
"routeHooks": null,
"redirect": null,
"relativeFilePath": "pages/AboutPage/AboutPage.tsx",
"isPrivate": true,
"unauthenticated": "home"
},
"/multi-cell": {
"name": "multiCell",
"bundle": null,
"matchRegexString": "^/multi-cell$",
"pathDefinition": "/multi-cell",
"hasParams": false,
"routeHooks": null,
"redirect": null,
"relativeFilePath": "pages/MultiCellPage/MultiCellPage.tsx",
"isPrivate": false
},
... |
Hm am seeing a Windows fail:
I wonder if the path I build now isn't compatible. But - the path in server entries would have assets, not the |
Fixed it :)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Leaving a couple of minor comments DT!
Thanks! Updated the RWPage const to constName and added tests. |
DT forgot to mention, totally OK in another PR! We will need to update the equivalent of "route manifest" on the dev side too, but should be straightforward! https://github.com/redwoodjs/redwood/blob/main/packages/internal/src/routes.ts#L86 |
Oh, I didn't know that existed in dev. Ok will do in a PR soon. I have been running and testing RSC with |
…-role-authmw * 'main' of github.com:redwoodjs/redwood: fix(dbAuthMw): Update and fix logic related to dbAuth "verbs" and decryptionErrors (redwoodjs#10668) RSC: routes-auto-loader is not used for SSR anymore (redwoodjs#10672) chore(crwa): Remove unused jest dev dependency (redwoodjs#10673) RSC: rscBuildEntriesFile: Only ServerEntry and Routes needed for serverEntries (redwoodjs#10671) RSC: clientSsr: getServerEntryComponent() (redwoodjs#10670) RSC: worker: getFunctionComponent -> getRoutesComponent (redwoodjs#10669) RSC: kitchen-sink: Make the ReadFileServerCell output take up less space (redwoodjs#10667) RSC: Remove commented code related to prefixToRemove transform() (redwoodjs#10666) RSC Client Router (redwoodjs#10557) RSC: Add 'use client' to remaining client cells in kitchen-sink (redwoodjs#10665) RSC: vite auto-loader: Spell out 'path' and other chores (redwoodjs#10662) fix(cli): Handle case for no arguments for verbose baremetal deploy (redwoodjs#10663) RSC: kitchen-sink: Make it more clear where layout ends and main content starts (redwoodjs#10661) RSC: Make the kitchen-sink smoke-test more robust/resilient (redwoodjs#10660) RSC: Source format of EmptyUsersCell in kitchen-sink (redwoodjs#10658) RSC: Add 'use client' to all client cells in kitchen-sink (redwoodjs#10659) chore(__fixtures__): Follow-up: Make test projects match newer CRWA template (redwoodjs#10657) feat: Reworks RSC server entries and route manifest building to derive from routes and include if route info related to authentication (redwoodjs#10572) chore(__fixtures__): Make test projects match newer CRWA template (redwoodjs#10655)
In furtherance if https://github.com/orgs/redwoodjs/projects/18/views/3?pane=issue&itemId=59049687
Currently @dac09 and I are thinking that to enforce authenticate during RSC rendering, we need to know if the entry request that renders the RSC component requires auth. This is not online how a graphQL operation is marked as requiring auth so that then the headers/cookie/credentials can be verified.
This PR refactors
How server entries are built -- not from "processing the pages dir" (which is a deprecated function) but rather the routes ... and the page info for that route. Note here that a page can be used in multiple routes, so the auth info cannot really be determined here.
The route manifest building to include an isPrivate attribute. Now if some page, route request is being handler we might be able to check if it "isPrivate" and enforce auth.
Add
unauthenticated
property to the route manifest to show where the user would expect to be redirected to if not permitted.Add
roles
to the route manifestBoth of these efforts are just a little speculation, but need the ability to check to see if our approach is reasonable.
FYI. The change in the RWRoute appears to have been an oversight from when the set was renamed from
Private
toPrivateSet
. Now a route properly knows if it's private based on its parents.Tests
Added to test to confirm that router can determine which routes are private. Note that fixtures are updated to favor
PrivateSet
overPrivate
... but the detection code still allows Private or PrivateSet.Tests added to check the name, path, unauthenticated and roles attributers of a RWRoute.
Examples
Entries
The
web/dist/rsc/entries.mjs
builds correctly after refactor and app runs.Route Manifest
builds .. notice the about routes are private.