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

embedded api #1637

Merged
merged 45 commits into from
Sep 10, 2024
Merged

embedded api #1637

merged 45 commits into from
Sep 10, 2024

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Sep 3, 2024

TODO:

  • Generate embed script (e.g., /chart.js) during preview
  • Generate embed script (e.g., /chart.js) during build via embedPaths dynamicPaths
  • Fix build so that module rendering applies content-hashed aliases (via resolvers)
  • Fix build so that assets & imports of embedded modules are included
  • Fix build so that the tree output is relevant for embedded modules
  • Fix messy types in build
  • Embed script script takes precedence over page (e.g., /chart.js.md)
  • Ignore pre-empted pages (/chart.js.md given /chart.js) in the sidebar & pager
  • Compute transitive static imports for preloading
  • Inline file registration to avoid ordering problem
  • Allow CORS during preview
  • Restrict CORS preview to specific origins
  • Documentation
  • Tests

Fixes #1084.
Fixes #1583.

@Fil Fil mentioned this pull request Sep 4, 2024
@mbostock
Copy link
Member Author

mbostock commented Sep 7, 2024

Would be simpler to just invert the precedence and have /chart.js.md take precedence over /chart.js? I don’t think it matters which one we choose; we just have to pick one, so we should pick whatever’s easier for us. Edit: There is a philosophical reason to give /chart.js precedence which is that it is an exact match and more specific than /chart.js.md, and hence we should stay the course. Further edit: I think we could just disallow /chart.js.md entirely and require that pages only have a single extension. That restriction should simplify things and feels reasonable and more forward-looking.

@mbostock
Copy link
Member Author

mbostock commented Sep 8, 2024

Boo, one of our tests has a page sub/page1..10.md which already violates the double extension rule, suggesting that we should allow pages to have double extensions but fix the precedence issue… I need to think more.

@mbostock mbostock requested a review from Fil September 10, 2024 02:19
@mbostock mbostock marked this pull request as ready for review September 10, 2024 02:19
Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fantastic! and I couldn't find how to break it 👍
Adding a few suggestions for the documentation.

docs/embeds.md Outdated Show resolved Hide resolved
docs/embeds.md Outdated Show resolved Hide resolved
```js run=false
import postgres from "postgres";

const sql = postgres(); // Note: uses psql environment variables
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const sql = postgres(); // Note: uses psql environment variables
const sql = postgres(); // See https://github.com/porsager/postgres to set up the environment variables

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment exists in multiple places. (I’ve been copying it every time I use Postgres.js.) I’m not sure we need it. The link from the text is probably enough, but I did think it was useful to mention that it depends on environment variables.

docs/embeds.md Outdated Show resolved Hide resolved
docs/embeds.md Outdated Show resolved Hide resolved
}, []);

return <div ref={ref} />;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to test this and log every line of it! it works :)


<div class="tip">

You can alternatively embed Framework pages using [iframe embeds](https://observablehq.observablehq.cloud/framework-example-responsive-iframe/).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking of which, an example of a responsive chart (that reruns to adapt to the container's width) might be welcome here, or separately in a full-blown example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. That should just be the resize helper, as normal.

src/preview.ts Show resolved Hide resolved
};
```

Or for [parameterized routes](./params), name the component `product-[id]/chart.js`, then load a list of product identifiers from a database with a SQL query:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit too allusive. It might be worth developing an example of how to use observable.params.id within chart.js so that it corresponds to the requested product and displays data from the corresponding (parametrized) data loader. The example can live as a stand-alone example app, and we'd just add a link here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I didn’t want to duplicate the documentation of parameterized routes here, but I also wanted to allude to it, since it feels like a natural question (how would I show multiple views?) with perhaps a surprising answer (the views bake-in their data rather than expecting it to be passed in as a prop from the host app).

Co-authored-by: Philippe Rivière <[email protected]>
docs/embeds.md Outdated Show resolved Hide resolved
@mbostock mbostock merged commit 7113770 into main Sep 10, 2024
4 checks passed
@mbostock mbostock deleted the mbostock/embedded branch September 10, 2024 17:32
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.

Add CORS headers in preview mode A JavaScript API for embedded use cases
2 participants