Initial system plugins #645
Conversation
James-Mart
force-pushed
the
initial-system-plugins
branch
from
c600734
to
f51feeb
Compare
| [dependencies] | ||
| bitflags = "2.4.2" | ||
| wit-bindgen-rt = "0.22.0" | ||
| #psibase = { version = "0.7.0", path = "../../../../rust/psibase/" } |
There was a problem hiding this comment.
Cruft? or left in here for some future use?
services/user/CommonSys/common/packages/common-lib/src/messaging/supervisor.ts
Show resolved
Hide resolved
| result: T; | ||
| const getFunctionBody = (pluginFunc: PluginFunc, resultCache: ResultCache[]): string => { | ||
| let {service, plugin, intf, method} = pluginFunc; | ||
| const found = resultCache.find((f: ResultCache)=>{ |
There was a problem hiding this comment.
Seems like prettier formatting is absent here
| message: `${toString(args)}: Failed to download ${service}:${plugin}.` | ||
| } | ||
| }); | ||
| } else if ( e instanceof ParserDownloadFailed ) { |
| return fetch_cache.get(url); | ||
| } | ||
|
|
||
| await sleep(200); |
There was a problem hiding this comment.
This is a local copy of the https://github.com/DougAnderson444/rollup-plugin-wit-component repo. I added a local copy so we could control the version of JCO instead of having to wait for Doug to publish a new version of his plugin. Furthermore we will eventually cache the transpilation in this plugin for some additional time savings.
I've now added attribution to the tops of all .js files in this /lib/ directory.
| let inviter = match inviter { | ||
| Some(i) => i, | ||
| None => { | ||
| return Err(InviterLoggedIn.err("")); | ||
| } | ||
| }; |
There was a problem hiding this comment.
We could use ok_or here hey?
There was a problem hiding this comment.
Well... I was going to say no, because I'm returning Err from the function (not assigning it to inviter).
Buuuut, actually the answer is yes. I can just use ? ...clever :)
Actually, ok_or_else (ok_or is apparently eagerly evaluated?)
There was a problem hiding this comment.
Fixed (and in general made all the error handling more idiomatic)
| try { | ||
| if (messageEvent.origin === supervisorOrigin) { | ||
| if (isIFrameInitialized(messageEvent.data)) { | ||
| this.onSupervisorInitialized(); | ||
| } else if (isFunctionCallResponse(messageEvent.data)) { | ||
| this.onFunctionCallResponse(messageEvent.data); | ||
| } | ||
| } else if (messageEvent.origin !== myOrigin) { | ||
| console.log("Received unauthorized message. Ignoring."); | ||
| } | ||
| } catch (e) { | ||
| if (e instanceof Error) { | ||
| console.error(e.message); | ||
| } else { | ||
| console.error( | ||
| `Unrecognized error: ${JSON.stringify(e, null, 2)}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Generally in js/ts, to avoid nested if statements and make this more readable, we'd use early returns:
if (messageEvent.origin !== myOrigin) {
console.log("Received unauthorized message. Ignoring.");
return;
}
try {
if (isIFrameInitialized(messageEvent.data)) {
this.onSupervisorInitialized();
} else if (isFunctionCallResponse(messageEvent.data)) {
this.onFunctionCallResponse(messageEvent.data);
}
} catch (e) {
if (e instanceof Error) {
console.error(e.message);
return;
}
console.error(`Unrecognized error: ${JSON.stringify(e, null, 2)}`);
}| const expected = this.pendingRequest!.call; | ||
| const received = response.call; | ||
| const unexpected: boolean = | ||
| expected.method != received.method || | ||
| expected.service != received.service; | ||
|
|
||
| if (isErrorResponse(response)) { | ||
| this.pendingRequest!.reject(response.result.val); |
There was a problem hiding this comment.
I would think the ! in this.pendingRequest! wouldn't be required in these cases...is TS complaining? It should know it exists because of your check on line 112 above. 🤔 Also, line 117...does it not infer the type?
There was a problem hiding this comment.
Yeah I was wondering that too.
There was a problem hiding this comment.
Nah ts wasn't complaining, just lack of familiarity with ts type deduction.
Fixed
| } else { | ||
| if (found.result !== undefined) { | ||
| if (isErrorResult(found.result)) { | ||
| return `throw ${JSON.stringify(found.result.val)};`; | ||
| } else { | ||
| return `return ${JSON.stringify(found.result)};`; | ||
| } | ||
| } else { | ||
| return ""; // No-op if the sync call has no return value. | ||
| } | ||
| } |
There was a problem hiding this comment.
I recommend dropping the else here on line 53. It's not needed because of the return in the preceding if condition on line 42. Then, to avoid further nesting:
if (found.result === undefined) {
return ""; // No-op...
}
if (isErrorResult(found.result)) {
return `throw ${JSON.stringify(found.result.val)};`;
}
return `return ${JSON.stringify(found.result)};`;| if (dupIndex !== -1) { | ||
| if (args !== actions[dupIndex].args) { | ||
| throw new Error("Mismatched args"); | ||
| } | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
const dupIndex = actions.findIndex((a) => a.action === action);
if (dupIndex === -1) {
return false;
}
if (args !== actions[dupIndex].args) {
throw new Error("Mismatched args");
}
return true;Or even:
const targetAction = actions.find((a) => a.action === action);
if (!targetAction) {
return false;
}
if (args !== targetAction.args) {
throw new Error("Mismatched args");
}
return true;| export const server = { | ||
| // add-action-to-transaction: func(service: string, action: string, args: list<u8>) -> result<_, string>; | ||
| addActionToTransaction(action, args) { | ||
| if (myService === UNINITIALIZED) |
There was a problem hiding this comment.
I'd add the brackets here for consistency. And there are a couple opportunities in this function to drop some else's too...but I'm getting pretty nitpicky at this point haha. Same goes for the rest of this file.
| if (!cache.exists(parserCacheKey)) { | ||
| let parserBytes: Uint8Array; | ||
| try { | ||
| parserBytes = await wasmUrlToIntArray( | ||
| "/common/component_parser.wasm" | ||
| ); | ||
| } catch (e) { | ||
| console.error("Loader: component_parser download failed"); | ||
| throw new ParserDownloadFailed(); | ||
| } | ||
| // Performance improvement: Target wasm32-unknown-unknown when compiling | ||
| // parser instead of wasm32-wasi, then loading can be simplified without wasi | ||
| // shims. | ||
| cache.cacheData(parserCacheKey, await load(parserBytes)); | ||
| return cache.getCachedData(parserCacheKey); | ||
| } else return cache.getCachedData(parserCacheKey); | ||
| }; |
There was a problem hiding this comment.
Can we flip this around to avoid more nesting?
if (cache.exists(parserCacheKey)) {
return cache.getCachedData(parserCacheKey);
}
let parserBytes: Uint8Array;
...There was a problem hiding this comment.
Same for the next couple functions.
There was a problem hiding this comment.
I guess I don't need to think about tail call optimization in js
There was a problem hiding this comment.
Changes to this file are only formatting changes, AND our default .prettier config for JS/TS files includes the trailing comma. Please checkout this file and restore it to pre-edit status.
There was a problem hiding this comment.
I just formatted according to the .prettierrc.json that already existed in common-lib. Where is this "default .prettier config" of which you speak? I'll update this prettier config to match.
There was a problem hiding this comment.
Ah, I see in another comment - I'll use the one in NftSys
There was a problem hiding this comment.
Fixed - I corrected the prettier configs and ran the updated config on each project: supervisor ui, wasm-loader, and common-lib
There was a problem hiding this comment.
Similar note here; please configure .prettier to match, for example, the NftSys .prettierrc.json, which is just
{
"tabWidth": 4,
"plugins": ["prettier-plugin-tailwindcss"]
}
| }; | ||
| } catch (err) { | ||
| throw new Error(err.message); | ||
| } |
There was a problem hiding this comment.
Any reason not to leverage something like the fetch api? Doing AJAX like this is old school :)
const resp = fetch(req.uri, {
method: req.method.toString(),
headers: new Headers(req.headers)
})
There was a problem hiding this comment.
Yes, the reason is that this magic allows it to be a synchronous import with no groundhog day!
| export default plugin; | ||
| import { plugin } from './plugin.js'; | ||
| import { load } from './loader.js'; | ||
| export { plugin, load }; |
There was a problem hiding this comment.
You can do this as follows (which is the standard)
export { plugin } from './plugin.js';
export { load } from './loader.js';
There was a problem hiding this comment.
James-Mart
force-pushed
the
initial-system-plugins
branch
from
8022aba
to
313c288
Compare
James-Mart
added
Plugin Infra
This adds some initial system app plugins and the initial component runtime infrastructure necessary for them to function. The plugins are only partially completed, and primarily intended to show the ergonomics of writing plugins.
The DemoApp1 front-end shows how to make calls to those plugins.