feat: generate edge function tarballs #6568
Conversation
|
This pull request adds or modifies JavaScript ( |
.github/workflows/workflow.yml
Outdated
| @@ -50,12 +50,12 @@ jobs: | |||
| os: [ubuntu-24.04, macos-14, windows-2025] | |||
| node-version: ['22'] | |||
| # Must include the minimum deno version from the `DENO_VERSION_RANGE` constant in `node/bridge.ts`. | |||
| deno-version: ['v1.39.0', 'v2.2.4'] | |||
| deno-version: ['v2.4.2'] | |||
There was a problem hiding this comment.
This is unfortunate, as it's the only part of this PR that affects users not targeted by the feature flag. But deno bundle isn't available in previous versions.
| const entries: string[] = [] | ||
|
|
||
| async function walk(currentPath: string): Promise<void> { | ||
| const dirents = await fs.readdir(currentPath, { withFileTypes: true }) |
There was a problem hiding this comment.
To be safe, we should sort the directory entries, this will help ensure we have a consistent hash between runs
| const dirents = await fs.readdir(currentPath, { withFileTypes: true }) | |
| const dirents = await fs.readdir(currentPath, { withFileTypes: true }).sort((a, b) => a.name.localeCompare(b.name)) |
There was a problem hiding this comment.
Done a little bit differently in 92603bb.
| } else if (dirent.isFile()) { | ||
| const fileHash = await getFileHash(fullPath) | ||
| entries.push(`${relativePath}:${fileHash}`) | ||
| } |
There was a problem hiding this comment.
If the package being bundled uses a package manager like pnpm which uses symbolic links, i think we will be missing those in the bundling with this current implementation. Perhaps we should treat symlinks and files the same like this?
| } else if (dirent.isFile()) { | |
| const fileHash = await getFileHash(fullPath) | |
| entries.push(`${relativePath}:${fileHash}`) | |
| } | |
| } else if (dirent.isFile() || dirent.isSymbolicLink()) { | |
| const fileHash = await getFileHash(fullPath) | |
| entries.push(`${relativePath}:${fileHash}`) | |
| } |
There was a problem hiding this comment.
I think we can actually remove the if entirely? Done in 92603bb.
There was a problem hiding this comment.
I'm not sure, that should likely work but dirents can be also be block devices, character devices, fifos, and sockets - i don't imagine people would be having those in the directories we are traversing but it is a possibility
There was a problem hiding this comment.
Good point. Added back in b1a719b.
| @@ -22,7 +22,7 @@ export const getDirectoryHash = async (dirPath: string): Promise<string> => { | |||
|
|
|||
| await walk(dirPath) | |||
|
|
|||
| return getStringHash(entries.join('\n')) | |||
| return getStringHash(entries.sort().join('\n')) | |||
There was a problem hiding this comment.
we might need to do a locale aware sort due to modern filesystems allowing pretty much any byte-sequence in a pathname
There was a problem hiding this comment.
But does that matter, since we don't really care where in the list each element appears as long as it always appears in the same place? I'm happy to add locale-aware sorting, I'm just trying to understand where the simpler solution will break.
There was a problem hiding this comment.
if we do not supply a comparator to Array.prototype.sort then it is implementation-defined, which means it could potentially not be a stable sort, which would lead to non-stable hashes being generated
step 5 of Array.prototype.sort which calls SortIndexedProperties and step 4 of that is the implementation-defined behaviour we would be invoking
Summary
Closes https://linear.app/netlify/issue/RUN-1790/generate-tarball-at-deploy-time.