Closed paulfitz closed 11 months ago
I pushed a preview of this to https://gristlabs.github.io/grist-static/next.html - should be usable from all the links that stay on that page.
Could probably do with a spinner - could be a delay if data engine hasn't started up yet and/or there's a lot of data - but I'd rather leave that to someone who is better at UI, and keep this PR limited to the plumbing job.
FWIW, a blob URL (rather than a service worker) is how I imagined implementing attachments.
I tried the preview just now, it worked nicely, but when I viewed products.csv and then downloaded, the filename was "null-Products.csv".
I tried the preview just now, it worked nicely, but when I viewed products.csv and then downloaded, the filename was "null-Products.csv".
Thanks @alexmojaki, I've made a tweak to csv-viewer to avoid setting the name of the document to the string "null" by default.
The whole MiniExpress thing is very impressive, but also scary. It seems very easy for an innocent-looking change in the internals of a grist endpoint to break grist-static. Here's how I think I'd have implemented it:
1. Split each downloading endpoint into three functions: 1. A central common function that contains as much of the actual logic as possible and is called by the other two. 2. A wrapper for grist-static which can be called without the context of express. Rather than working with requests and responses, it accepts simple arguments (e.g. `docId: string, template: boolean, ...`) and returns a `DownloadInfo`. It may need to construct little fake request objects for cases where inner logic is tightly coupled with requests. 3. An express endpoint which parses the request, calls the common function, and sends a response. 2. Where `maybeModifyLinkAttrs` is used, add a data attribute or two to the link containing the name of a function to call and the arguments to pass to it. So the link will contain the same information about the request it wants to make in two different forms. If we're feeling fancy, we could make a type-safe system that makes it less likely that the two forms will get out of sync. 3. Make `fetchAndDownload` call the correct wrapper function as directly as possible using the arguments stored on the element, rather than going through anything express-like.
What do you think? Is it possible? Is it worth changing at this stage?
(some of my comments below wouldn't apply in this case, I wrote them before I reached the point where I was seeing the bigger picture)
Your suggestions make sense. I had considered more of a refactor of grist-core, to disentangle functionality from endpoints. It makes logical sense, but it seemed a bit of a thicket to whack through. That'd be fine if grist-static were enough of a priority. It hasn't been in the past, which is why the grist-static repo is so YOLO in nature (no tests, lots of use of webpack resolution to stub things out, no release workflow), and written with minimal changes to grist-core. So I went a different route that felt like it could liberate a lot of DocApi endpoints with less work overall, while definitely (like a lot of grist-static) leaving the project vulnerable to changes in core.
Defer to @dsagal on this - what do you think Dmitry, is it time for grist-static to grow up?
I'd be glad to see it grow up a little, yes 🙂
This uses some new hooks being added in https://github.com/gristlabs/grist-core/pull/665 to enable csv, xlsx, and .grist downloads.
An alternative approach to reroute requests to front-end code would have been a service worker. For downloads, a tiny little hook in grist-core was sufficient, so I went with that for simplicity. It may be that we'll need a service worker later, at which point the plumbing could be changed a little.