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

page fragment loaders #1807

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

page fragment loaders #1807

wants to merge 5 commits into from

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Nov 9, 2024

This adds a server option to fenced code blocks. When present, the code is run on the server when the page is rendered in the same fashion as a data or page loader; the interpreter is passed the source of the fenced code block as stdin. The stdout of a page fragment loader must be HTML, which then replaces the comment placeholder (e.g., <!--:85902a01:-->).

Screenshot 2024-11-09 at 9 40 39 AM

Marking this as a draft because I haven’t thought through how caching should work. Currently page fragment loaders run every time the page is loaded, but I think they should only run when the page is edited, similar to page and data loaders. That means we’ll need to cache the output of page fragment loaders (or more likely the HTML of the entire page).

I also think the implementation could be cleaned up slightly, for example by moving the rendering of page fragments into a function, and by exposing the normalized interpreters on the config instead of on loaders… or maybe the loaders object should have a method for evaluating fragments.

Fixes #234. Probably also fixes #145, which we should consider duplicate.

@mbostock mbostock requested a review from Fil November 9, 2024 17:44
@Fil
Copy link
Contributor

Fil commented Nov 10, 2024

It's immensely satisfying to run DuckDB (from sh) and bake-in some statistics without having to write an external data loader. Makes it easier to stay in your train of thought as you're developing.

A few observations as I'm testing this:

  • How can we pass the page params to the fragment loader?

  • Crashes in the code block are fatal (we should instead surface the error).

  • It would be super nice to give a name to the output and make it available as a reactive variable, similar to what we do with sql blocks with id= (except here the value would be the DOM node). If I add an id manually then use querySelector(#id) I can work something in build, but it's a bit tedious and doesn't work in preview.

  • I think it would make sense to cache the fragment outputs aggressively, maybe based on a hash of the code (& interpreter). It would be faster to play with, and closer to the build (where date is the build date, not the current date).

  • I'm not always seeing hot-reloading; related, or not, with the following content I'm getting a loop of page reloads (which sometimes stops after a while):

```js server echo
console.log(Math.random());
```

showing ↓ { type: 'reload' } messages. Same with new Date(). (I don't understand how this can even be possible.)

@mbostock
Copy link
Member Author

I added support for params to page fragment loaders in f5b5a13. This required passing a couple additional arguments to the interpreter so that you can run commands from stdin while passing additional arguments to the script. In the case of sh, it looks like:

sh -s -- --theme=air

Whereas for Node:

node --no-warnings=ExperimentalWarning -- --theme=air

We should allow the config to define the arguments used when the interpreter is invoked, so we’ll probably need to bifurcate the interpreters option to distinguish between the interpreter being invoked with a file versus being invoked with stdin. Or maybe we should promote the page fragment source to a temporary file and use that, rather than using stdin. That’s one less thing for users to worry about.

@mbostock
Copy link
Member Author

Crashes in the code block are fatal (we should instead surface the error).

Maybe… I think the build should crash if a page fragment loader errors, in the same fashion as a page loader.

It would be super nice to give a name to the output and make it available as a reactive variable

I’ve been thinking about making HTML the base representation rather than Markdown. So, Markdown pages would first compile to HTML, but a flavor of HTML (”Observable HTML” say) that allows us to define <script type="observable">. And Observable HTML would be compiled into vanilla HTML as today. I originally thought page fragment loaders could emit Markdown rather than HTML, which could do something similar; but that would also allow page fragment loaders to emit page fragment loaders recursively. Whereas if HTML is the base representation, page fragment loaders are only expressible in Markdown and thus cannot be recursive.

Anyway, my goal with this PR is to introduce a minimal abstraction that serves as a foundation — to not add more than we need. We can add more in the future.

A pattern that I like from oss-analytics is defining JSON scripts:

https://github.com/observablehq/oss-analytics/blob/937a8af225c839c25b48ebfba3953a49a7774770/src/%40%5Bscope%5D/%5Bname%5D.md.js#L305-L308

These are then exposed as top-level variables using JSON.parse:

https://github.com/observablehq/oss-analytics/blob/937a8af225c839c25b48ebfba3953a49a7774770/src/%40%5Bscope%5D/%5Bname%5D.md.js#L95-L98

I think it would make sense to cache the fragment outputs aggressively, maybe based on a hash of the code

Maybe. I was thinking it should be based on the modification time of the file like we do with page & data loaders. That would mean that all page fragment loaders re-run when you edit the Markdown, but if we use hashes, it’s harder to force a re-evaluation. (And we don’t trace transitive dependencies of loaders, so sometimes they need a nudge.) That said, we use hashing for client-side code blocks, so it makes sense to have similar behavior for server-side code blocks, so you’re probably right!

I'm not always seeing hot-reloading; related, or not, with the following content I'm getting a loop of page reloads

The loop I expect if the loader is nondeterministic, and maybe is related to the other problem. It’s because we don’t have any caching. When the client connects, the first thing it does is send the hash of the page, and if it doesn’t match what the preview server’s hash, the client reloads the page. If you have a nondeterministic page fragment loader, since there is no caching, the hash will never be consistent and will reload continuously. Let’s implement caching before investigating this further.

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.

Server-side JavaScript Generate a data loader file from a cell
2 participants