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

fix: preserve mtime when zipping with the node zipper #539

Merged

Conversation

iainlane
Copy link
Contributor

@iainlane iainlane commented May 25, 2024

This means it's possible for code to retrieve the correct mtime, for example to return in a Last-Modified header. That's what I want to use this for: I'm bundling static assets in my Lambda and I want to be able to have clients cache them properly and send me If-Modified-Since.

Since we have a test which verifies a checksum of a zip file created by this function, we can be sure the results are deterministic. To support this, update the test inputs to have specified mtimes.

Here's a test run on my fork. You can see that I had to drop node 14 from CI, so if a run is approved it'll fail. This seems to be a pre-existing failure on master which you can observe in other PRs.

This means it's possible for code to retrieve the correct mtime, for
example to return in a `Last-Modified` header.

Since we have a test which verifies a checksum of a zip file created by
this function, we can be sure the results are deterministic. To support
this, update the test inputs to have specified mtimes.
@iainlane iainlane force-pushed the iainlane/zip-use-mtime-from-filesystem branch from 8446c7a to 9c5cbf2 Compare May 27, 2024 08:21
iainlane added a commit to iainlane/coldoutsi.de that referenced this pull request May 27, 2024
`serverless-esbuild`, which we use to build the project, doesn't preserve file
modification times. We make use of these times to generate the `last-modified`
headers for static files, which we bundle in the Lambda `.zip`s.  I [submitted a
PR upstream][pr] to fix this. Switch to using a fork containing this fix.

The `index` handler was implementing very similar logic to the `static` handler,
but slightly differently. We move the `index` handler to the `static` module and
make it use the same `fileData` as the `static` handler. This allows us to
remove the `index` handler entirely.

Add tests for both handlers. This uses a test file stored in the repo. But note
that `git` doesn't preserve file modification times, so we have to work these
out dynamically in the tests.

Fix a nit too: handlers don't have to be async. They can simply return their
result directly, no need to wrap it in a `Promise.resolve`.

[pr]: floydspace/serverless-esbuild#539
iainlane added a commit to iainlane/coldoutsi.de that referenced this pull request May 27, 2024
`serverless-esbuild`, which we use to build the project, doesn't preserve file
modification times. We make use of these times to generate the `last-modified`
headers for static files, which we bundle in the Lambda `.zip`s.  I [submitted a
PR upstream][pr] to fix this. Switch to using a fork containing this fix.

The `index` handler was implementing very similar logic to the `static` handler,
but slightly differently. We move the `index` handler to the `static` module and
make it use the same `fileData` as the `static` handler. This allows us to
remove the `index` handler entirely.

Add tests for both handlers. This uses a test file stored in the repo. But note
that `git` doesn't preserve file modification times, so we have to work these
out dynamically in the tests.

Fix a nit too: handlers don't have to be async. They can simply return their
result directly, no need to wrap it in a `Promise.resolve`.

[pr]: floydspace/serverless-esbuild#539
@floydspace floydspace merged commit 162d9bc into floydspace:master Sep 24, 2024
Copy link

🎉 This PR is included in version 1.54.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to 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.

2 participants