-
-
Notifications
You must be signed in to change notification settings - Fork 14
src: fix symlink issue, subtitution middleware hack #223
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
Conversation
bef9f45 to
019f2cf
Compare
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 assume this is the built file?
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.
Any reason why this cannot be inside the cachedDirectories folder? To reuse same logic and reduce the need of maintaining two different files and have centralised logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but it would mean that we'd need to store symlinks separately from the real files (something like
interface CachedDirectory {
subdirectories: string[],
symlinkSubdirectories: string[],
files: File[],
symlinkFiles: File[]
}).
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.
Very interesting. I wonder if this will be cached between workers so that it doesn't need to be read on every network request? I also wonder if somehow this gets cached or whatnot 🤔
Because I assume this file will keep increasing over time. We might want to consider using Git LFS to also prevent these files to be shown on diff.
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 wonder if this will be cached between workers
As in the contents of cachedDirectories.json? It's inlined along with the rest of the source files for the worker, so it's stored in-memory for each instance of the worker.
src/providers/r2Provider.ts
Outdated
| // of it below | ||
| const result: ReadDirectoryResult = CACHED_DIRECTORIES[path]; | ||
|
|
||
| for (const file of result.files) { |
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.
Instead of this, can you do this?:
const result = JSON.parse(input, function(key, value) {
if (key == 'date') return new Date(Date.parse(value));
return value;
});
Or something of sort? This speeds up the process and reduces the need of iterating everything again
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.
Since CACHED_DIRECTORIES is already an object I don't think so?
ovflowd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a first-pass comment, but it looks good so far. I left a few comments, and I plan to make another in-depth review later!
ovflowd
left a comment
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.
2nd pass. We're good to merge this PR!
Signed-off-by: flakey5 <[email protected]>
Signed-off-by: flakey5 <[email protected]>
Signed-off-by: flakey5 <[email protected]>
Main changes:
build-r2-symlinks.mjsscriptupdate-redirect-links.json, still createslatestVersion.jsonsrc/constants/docsDirectory.json- cache of folders that exist innodejs/docs/so we can access them in the worker w/o the latency of another R2 callsrc/constants/cachedDirectories.json- cache of the directory listings fornodejs/release/andnodejs/docs/with the symlinks addedMisc changes:
promote-release.mjs(unused)compile-handlebars.jsto be an .mjs fileRequest.unsubstitutedUrlFixes #163