[skip] Add reactive sessions. #792
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Just to make the docs generator able to find it.
They are an internal implementation detail.
There does not seem to be a way to hide this implementation detail in the documentation. So this change comments it out entirely, since the implementation is based on checking the sknative property, and the NativeStub superclass is surface-only.
- import Nullable from core since it is documented there Just to make the docs generator able to find it. - hide sknative properties in docs They are an internal implementation detail. - hide `implements NativeStub` There does not seem to be a way to hide this implementation detail in the documentation. So this change comments it out entirely, since the implementation is based on checking the sknative property, and the NativeStub superclass is surface-only.
They were broken by SkipLabs#401 See this PR for an example of correctly triggered jobs
Much better than just `CRuntimeError(1)`
A few cleanups while trying to fix the CI on SkipLabs#766...
It looks like the only benefit is to be able to write k => v in for loops but it makes it harder to reason about the linear usage of iterators
because values are never used
I don't see the point in keeping this wrapper, it makes reasoning about iterators a bit harder. And we don't even use the values
...in `populateDefinitions`, as revealed by SkipLabs#771
- Simplify script - Reduce special cases - Improve dependencies (not reading Skargo.toml/package.json yet) - At least build skip packages that have no tests
I noticed while adding an optional default parameter to Values#getUnique that it's probably a footgun to export `sk_freeze` from skjson via `core`'s index.ts, since it doesn't actually _freeze_, just adds the hidden property that we use to track frozen-ness. This PR drops that export and renames to avoid confusion.
Using a codeql.yml rather than the default setup
- Using a codeql.yml rather than the default setup - keep the weekly analysis only - ignore sql/
jberdine
reviewed
Mar 14, 2025
skipruntime-ts/server/src/rest.ts
Outdated
Comment on lines
117
to
127
| app.put("/v1/sessions/:session_id", (req, res) => { | ||
| try { | ||
| service.update("__sessions", [ | ||
| [req.params.session_id, [req.body as Json]], | ||
| ] as Entry<string, Json>[]); | ||
| res.sendStatus(200); | ||
| } catch (e: unknown) { | ||
| console.log(e); | ||
| res.status(500).json(e instanceof Error ? e.message : e); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The data associated with each session-id is an opaque json blob as far as skip is concerned, right? But if someone wants to use it as a key-value store, they have to set the whole thing at once. Since there is no way to get the current value to use as the basis of an extension, e.g. to add a new key-value association, then anyone wanting to call PUT needs to themselves keep track of the whole thing. I don't know if that fits the expected uses. Would it be good to add a GET method to enable extending rather than only overwriting the session data?
skipruntime-ts/core/src/api.ts
Outdated
| @@ -318,6 +318,8 @@ export interface LazyCompute<K extends Json, V extends Json> { | |||
| * Skip Runtime internal state. | |||
| */ | |||
| export interface Context extends Managed { | |||
| readonly session: (Json & DepSafe) | null; | |||
There was a problem hiding this comment.
One important aspect is just when it is null vs non-null.
The checked in bootstrap `.ll.gz` files contain absolute paths that only make sense for the person building them. Paths are found in `!DIFile` directives (both absolute CWD `directory` and absolute file path `filename`) and other positions (e.g. those found in messages for unreachableMethodCall calls). This PR: - makes `!DIFile`'s `filename` relative to the `directory`, _all the time_, from what I could find, this seems legit - makes all file names relative to `CWD`, **if** `--canonize-paths` is given Hence, to compile `skc` with canonized paths, use `skargo build --skcopt --canonize-paths`
This commit makes a `context.session` object available to resources. It works by implicitly adding a `__sessions` input collection to services, which maps `string`s to arbitrary `Json`, and using the `Session-Id` HTTP header passed to the server's read end points to inject the required session into the context. Sessions can be manipulated via two new control end points: - `PUT /v1/sessions/:session_id`, - `DELETE /v1/sessions/:session_id`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit makes a
context.sessionobject available to resources. It works by implicitly adding a__sessionsinput collection to services, which mapsstrings to arbitraryJson, and using theSession-IdHTTP header passed to the server's read end points to inject the required session into the context.Sessions can be manipulated via two new control end points:
PUT /v1/sessions/:session_id,DELETE /v1/sessions/:session_id.