lastModified #1051
lastModified #1051
Conversation
| registerFile("./static-tar/does-not-exist.txt", {"name":"./static-tar/does-not-exist.txt","mimeType":"text/plain","path":"./static-tar/does-not-exist.txt"}); | ||
| registerFile("./static-tar/file.txt", {"name":"./static-tar/file.txt","mimeType":"text/plain","path":"./_file/static-tar/file.c93138d8.txt"}); | ||
| registerFile("./static-tgz/file.txt", {"name":"./static-tgz/file.txt","mimeType":"text/plain","path":"./_file/static-tgz/file.c93138d8.txt"}); | ||
| registerFile("./static-tar/does-not-exist.txt", {"name":"./static-tar/does-not-exist.txt","mimeType":"text/plain","path":"./static-tar/does-not-exist.txt","lastModified":/* ts */1706742000000}); |
There was a problem hiding this comment.
This file does not exist BUT it has a source, so it gets a lastModified; a bit weird maybe. Contrast it with a file that does not exist at all (no file, no data loader, no archive to extract from): in that case we (correctly) don't send a lastModified property:
in test/output//build/missing-file/index.html:
registerFile("./does-not-exist.txt", {"name":"./does-not-exist.txt","mimeType":"text/plain","path":"./does-not-exist.txt"});
Co-authored-by: Mike Bostock <mbostock@gmail.com>
|
I've changed the logic and added a few tests. |
src/client/stdlib/fileAttachment.js
Outdated
| @@ -28,9 +28,10 @@ async function dsv(file, delimiter, {array = false, typed = false} = {}) { | |||
| } | |||
|
|
|||
| export class AbstractFile { | |||
| constructor(name, mimeType = "application/octet-stream") { | |||
| Object.defineProperty(this, "name", {value: `${name}`, enumerable: true}); | |||
| constructor(name, mimeType, lastModified) { | |||
There was a problem hiding this comment.
Looks like you (inadvertently?) dropped the default for mimeType here. I think we want to keep that (and I’m surprised there’s not a test). Also this is part of a public API so we wouldn’t want to remove the default in any case.
There was a problem hiding this comment.
Oops, good catch. I'll add a test!
There was a problem hiding this comment.
Ah now I remember—I wanted to change this so that it would be defined in the call to registerFile instead of in the client.
So, in render.ts we'd have:
- mimeType: mime.getType(name) ?? undefined,
+ mimeType: mime.getType(name) ?? "application/octet-stream",and then we don't need this default in the client (and it becomes possible to do a "build test").
There was a problem hiding this comment.
I've done that now; we can roll back and add the default to client/stdlib, but then I don't know how to test.
There was a problem hiding this comment.
Yes, please move the default back to the client.
| Object.defineProperty(this, "mimeType", {value: `${mimeType}`, enumerable: true}); | ||
| Object.defineProperty(this, "name", {value: `${name}`, enumerable: true}); | ||
| if (lastModified !== undefined) Object.defineProperty(this, "lastModified", {value: Number(lastModified), enumerable: true}); // prettier-ignore |
There was a problem hiding this comment.
Fine to always include this property, even if undefined?
There was a problem hiding this comment.
We still need a test because we don't want to show NaN for an undefined value?
|
With this architecture I think we'll be able to do even better, for example by adding the file size (if known), image dimensions for common formats, etc. |
Adds a lastModified property to registered files.
This property is computed as the modification time of, in order:
On build, this happens when rendering the pages, which is after the (stale) data loaders have run. This means that the time is consistently given by 1 or 2—and we get to 3 only in the rare case where the data loader has failed to produce a cached artifact.
In preview, by contrast, this happens before the data loader has had a chance to run. So, when the cache is stale, you get stale information—but the situation solves itself on the next page load. (It is not possible to solve this in any other way as we can't predict the future, and don't want to wait for the loader to have finished running before we send the page to the browser.)
At the same time and with the same logic (1, 2, 3), the hash of a file generated by a data loader is now based on the cached result rather than on the data loader code.
closes #62
supersedes #697
supersedes #699
Notes:
Follows the same pattern as
File.lastModified(a number not aDatebecause numbers are immutable).Does not use
touchfor unit tests, so only checks that there is (or isn't) a timestamp.