-
Notifications
You must be signed in to change notification settings - Fork 35
Add React Dataflow example using dynamic import #22
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
Conversation
behavior is identical to https://observablehq.github.io/examples/react-dataflow/ (oops gotta update the styles in my new one) |
oh i could also strip out the create-react-app junk and use rollup or… |
react-dataflow-dynamic/README.md
Outdated
This version imports the Observable Runtime and the notebook dynamically, so that you’ll always see the latest published version of the notebook when you load the page, instead of having to deploy a new version of your application. This also sidesteps the issues some bundlers have with Observable file attachments. Contrast with [static import](https://github.com/observablehq/examples/tree/main/react-dataflow/). | ||
|
||
```js | ||
const {Runtime, Inspector} = await import("https://cdn.jsdelivr.net/npm/@observablehq/runtime@4/dist/runtime.js"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the user can choose to install the runtime or to use this... can we perhaps have a comment here that suggests what to change if they want to install the runtime instead of dynamically loading the runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah true
How about we call the existing method "Runtime with React (downloaded)" and add another type called "Runtime with React (dynamic)"? |
(gotta switch to some other work, will come back to this maybe tonight & rip out create-react-app so it's not 12k lines of package-lock lol) |
return () => { | ||
setModule(undefined); | ||
runtime.dispose(); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This clean up isn't returned to useEffect because of the async…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do something like this:
useEffect(() => {
async function load() {
await stuff;
return runtime;
}
const promise = load();
return () => {
promise.then((runtime) => {
setModule(undefined);
runtime.dispose();
});
};
}, []);
But even that’s not quite right because it’s (theoretically) possible that a second invocation of the effect could have its promise resolve first, which might result in the first effect’s invalidation clearing the module after the second effect was initialized.
Another consideration is that ref.current
could be undefined (or null?) if the component is destroyed before the dynamic imports resolve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adopted that pattern for now ^^
"a second invocation of the effect…" — i thought the empty dependency array meant it'd only run once? i guess it doesn't guarantee that?
"ref.current could be undefined…" — so the custom observer could check if ref.current is truthy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this also work?
useEffect(() => {
let dispose = () => {}
(async () => {
const { default: notebook } = await import(notebookUrl)
const runtime = new Runtime()
const module = runtime.module(notebook)
dispose = d => runtime.dispose()
})()
return () => dispose()
}, [])
const {Runtime, Inspector} = await import("https://cdn.jsdelivr.net/npm/@observablehq/runtime@4/dist/runtime.js"); | ||
const {default: notebook} = await import("https://api.observablehq.com/@d3/brushable-scatterplot.js?v=3"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You’ll want to parallelize these using Promises.all:
const {Runtime, Inspector} = await import("https://cdn.jsdelivr.net/npm/@observablehq/runtime@4/dist/runtime.js"); | |
const {default: notebook} = await import("https://api.observablehq.com/@d3/brushable-scatterplot.js?v=3"); | |
const [ | |
{Runtime, Inspector}, | |
{default: notebook} | |
] = await Promise.all([ | |
import("https://cdn.jsdelivr.net/npm/@observablehq/runtime@4/dist/runtime.js"), | |
import("https://api.observablehq.com/@d3/brushable-scatterplot.js?v=3") | |
]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i changed to install the runtime statically at compile time and only import notebook at runtime… but i will remember this in the future!!
Previous create-react-app version of the PR: +11,923 lines of code Now just adapting the rollup config from react-dataflow example: +1,039 lines of code 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A user asked Cobus if you could embed a notebook in React such that it fetches the latest published version of the notebook at runtime, instead of installing and checking in and bundling and having to redeploy if you make changes.
This also sidesteps the file attachment bundler issues! Wondering if it should be the default technique we recommend in the Embed modal.