Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

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

src: map file symlinks for old node versions #292

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

flakey5
Copy link
Member

@flakey5 flakey5 commented Feb 10, 2025

Old Node versions have file symlinks pointing to them, but the worker wasn't handling this (i.e. for https://nodejs.org/dist/v0.1.99/ there should be a node-v0.1.99.tar.gz file but there's not).

This:

  • Makes a new constants file fileSymlinks.json that holds whatever files need symlinks. The key is the symlink location and the value is the actual location of the file
  • Makes the build-r2-symlinks.js script read fileSymlinks.json and include them in the cached directory listings
  • Makes the R2 provider handle these symlinks

@flakey5 flakey5 requested a review from a team as a code owner February 10, 2025 20:40
Copy link
Member Author

Choose a reason for hiding this comment

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

We're still well under the 10mb limit for a worker but since this file is only going to grow, I think we should continue looking at something like #159

Copy link
Member

Choose a reason for hiding this comment

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

Agreed :)

// Let's add these to our cached directories.
const fileSymlinks = JSON.parse(await readFile(FILE_SYMLINKS, 'utf8'));

for (const file of Object.keys(fileSymlinks)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can this be cleaned up a bit? Maybe instead if else, we can have different loops just to separate concerns. Performance here should not be a driving factor.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not really sure separating it into two different loops would look much better,

for (const file of Object.keys(fileSymlinks)) {
  const directory = `${dirname(file)}/`

  if (!(directory in cachedDirectories)) {
    continue;
  }

  const actualFile = await headFile(client, fileSymlinks[file]);

  cachedDirectories[directory].files.push({
    ...actualFile,
    name: basename(file)
  })
}

for (const file of Object.keys(fileSymlinks)) {
  const directory = `${dirname(file)}/`

  if (!(directory in cachedDirectories)) {
    continue;
  }

  const actualFile = await headFile(client, fileSymlinks[file]);

  const contents = await listDirectory(client, directory);
  contents.files.push({
    ...actualFile,
    name: basename(file),
  })

  cachedDirectories[directory] = contents;
}

@flakey5 flakey5 force-pushed the flakey5/20250128/symlinks branch from 6b08c2e to 71edd3a Compare February 21, 2025 02:01
@flakey5 flakey5 merged commit 114c261 into main Feb 21, 2025
5 checks passed
@flakey5 flakey5 deleted the flakey5/20250128/symlinks branch February 21, 2025 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants