-
Notifications
You must be signed in to change notification settings - Fork 71
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
v6; more modular #374
v6; more modular #374
Changes from 10 commits
6db069b
7c222bf
ee8dc2f
6bf630d
fb80e6a
d7afbb0
baadd7a
2f36172
ddd3d48
677e368
e6fbbc2
922eadd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
name: Publish | ||
|
||
on: | ||
workflow_dispatch: {} | ||
release: | ||
types: [published] | ||
|
||
jobs: | ||
publish: | ||
runs-on: ubuntu-latest | ||
permissions: | ||
contents: read | ||
packages: write | ||
steps: | ||
- uses: actions/checkout@v4 | ||
- uses: actions/setup-node@v4 | ||
with: | ||
node-version: 20 | ||
cache: yarn | ||
- run: yarn --frozen-lockfile | ||
- run: yarn test | ||
- run: npm publish | ||
env: | ||
NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
name: Test | ||
|
||
on: | ||
push: | ||
branches: [main] | ||
pull_request: | ||
branches: [main] | ||
|
||
jobs: | ||
test: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v4 | ||
- uses: actions/setup-node@v4 | ||
with: | ||
node-version: 20 | ||
cache: yarn | ||
- run: yarn --frozen-lockfile | ||
- run: yarn test |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,4 @@ | ||
*.sublime-workspace | ||
.DS_Store | ||
.esm-cache/ | ||
dist/ | ||
node_modules | ||
npm-debug.log |
Large diffs are not rendered by default.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,2 @@ | ||
export {Inspector} from "@observablehq/inspector"; | ||
export {Library} from "@observablehq/stdlib"; | ||
export {RuntimeError} from "./errors.js"; | ||
export {Runtime} from "./runtime.js"; |
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
import {Runtime} from "@observablehq/runtime"; | ||
import assert from "assert"; | ||
import {sleep} from "../variable/valueof.js"; | ||
|
||
describe("runtime.dispose", () => { | ||
it("invalidates all variables", async () => { | ||
const runtime = new Runtime(); | ||
const main = runtime.module(); | ||
const log = []; | ||
main.variable(true).define(["invalidation"], async (invalidation) => { | ||
await invalidation; | ||
log.push("invalidation"); | ||
}); | ||
await sleep(); | ||
runtime.dispose(); | ||
await sleep(); | ||
assert.deepStrictEqual(log, ["invalidation"]); | ||
}); | ||
it("terminates generators", async () => { | ||
const runtime = new Runtime(); | ||
const main = runtime.module(); | ||
const log = []; | ||
main.variable(true).define([], function* () { | ||
try { | ||
while (true) yield; | ||
} finally { | ||
log.push("return"); | ||
} | ||
}); | ||
await sleep(); | ||
runtime.dispose(); | ||
await sleep(); | ||
assert.deepStrictEqual(log, ["return"]); | ||
}); | ||
}); |
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 change means that when a variable is redefined, it will wait for the old version of the variable to settle (fulfill or reject) before running. However, there are some important nuances here…
First, variables reject before computing if they are stale (
if (variable._version !== version)
). This means that if a variable is redefined multiple times, we won’t waste time computing already-invalidated definitions. Second, new variables are created when code is edited rather than redefining existing variables; see Framework’sdefine
, and note its call tomain.variable
to define a new variable whenever code is edited. This means that new (edited) code will not wait for old code to settle before running. However, unchanged code that is downstream of the edited code will still wait for the old code to settle, which might be confusing.We could fix the last point in Framework by redefining any downstream code as new variables if it proves to be confusing. As a workaround, you can reload the page if you don’t wait for old code to run.
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 delay is OK (as in the case of awaiting a now-useless SQL query), but it is definitely confusing when the unchanged code is not settling at all.
A typical example is when I make a mistake writing a cell as a Promise that never resolves; fixing the mistake in the code block does not fix the state of the page.
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.
So what do you suggest we do?
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.
After playing a bit more with it, only the programmer error case mentioned above can be seen as a regression, ending up in a situation where you have to reload the page to resync the tree of variables with your code — whereas in main fixing the code would fix it.
As for suggestion, I wish I had an idea :) “Redefining any downstream code as new variables”, as you mentioned, maybe with an additional timeout(?).
But the additional complexity might not be worth it. And this is such a big win that if we have to choose I prefer this version.
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.
We could make it so that calls to variable.define cause the variable and any downstream variables not to wait, such that we’d only end up waiting in the case of generators yielding a new value. I can look to see how hard that will be.