ubiquity-os / ubiquity-os-kernel

1 stars 19 forks source link

SDK #64

Closed whilefoo closed 3 months ago

whilefoo commented 4 months ago

@gentlementlegen I moved SDK code here but testing with bun test fails, even though tests passed in the SDK repo. What I'm suspecting is that bun test is run with Bun runtime which doesn't have Node.js crypto module fully supported.

I've tried switching to node.js/jest but I'm getting an error:

 Jest encountered an unexpected token

    Jest failed to parse a file. This happens e.g. when your code or its dependencies use non-standard JavaScript syntax, or when Jest is not configured to support such syntax.

    Out of the box Jest supports Babel, which will be used to transform your files into valid JS based on your Babel configuration.

    By default "node_modules" folder is ignored by transformers.

    Here's what you can do:
     • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/ecmascript-modules for how to enable it.
     • If you are trying to use TypeScript, see https://jestjs.io/docs/getting-started#using-typescript
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/configuration
    For information about custom transformations, see:
    https://jestjs.io/docs/code-transformation

    Details:

    ubiquity/ubiquibot-kernel/node_modules/@octokit/core/dist-src/index.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){import { getUserAgent } from "universal-user-agent";
                                                                                      ^^^^^^

    SyntaxError: Cannot use import statement outside a module

    > 1 | import { Octokit } from "@octokit/core";
        | ^
      2 | import { RequestOptions } from "@octokit/types";
      3 | import { paginateRest } from "@octokit/plugin-paginate-rest";
      4 | import { restEndpointMethods } from "@octokit/plugin-rest-endpoint-methods";

As far as I understand ts-jest should transpile Typescript to CommonJS module according to their docs so this should work.

whilefoo commented 4 months ago

After trying for so long, it seems the issue is only with octokit packages...when I removed them the test passed. Still trying to figure out how to fix it. I'm guessing it doesn't transform octokit packages to CommonJS, but even after configuring jest to transform everything, it still doesn't work

whilefoo commented 4 months ago

So after switching to esbuild-jest and including node_modules in the transform process, there was only one issue left:

Cannot find module '@octokit/webhooks-methods' from 'node_modules/@octokit/webhooks/dist-bundle/index.js'

I'm not sure why it can't find it but I fixed it by adding it to jest config:

moduleNameMapper: {
    "@octokit/webhooks-methods": "<rootDir>/node_modules/@octokit/webhooks-methods/dist-node/index.js",
}
whilefoo commented 4 months ago

I had to switch to swc because of this error https://github.com/aelbore/esbuild-jest/issues/57

@gentlementlegen it seems that @octokit/webhooks are not getting mocked and it's calling the real implementation. do you have any idea how to fix this?

gentlementlegen commented 4 months ago

@whilefoo my guess is that Jest hoists the module mock, so it doesn't use the mock but the real implementation instead. Could you try to do const worker = require("../src/worker").default; inside the test itself instead of importing at the top of the file?

whilefoo commented 4 months ago

This PR adds support for Worker plugins, it uses Hono framework which can run on any runtime (cloudflare/netlify/node.js) and is basically an API server that handles requests from the kernel, checks for signature, sets up the context (octokit, settings, env, payload) and calls the function provided by the plugin developer and returns the result from the function back to the kernel

gentlementlegen commented 4 months ago

@whilefoo How is it meant to be used exactly? I see no script in the package.json.

whilefoo commented 3 months ago

@whilefoo How is it meant to be used exactly? I see no script in the package.json.

This is example usage:

type Config = { test: boolean };

type Env = { test: string };

type SupportedEvents = "issue_comment.created";

const app = createPlugin(async (context: Context<Config, Env, SupportedEvents>) => {
  context.logger.info("Hello world");
  return { success: true };
});

export default app;
gentlementlegen commented 3 months ago

@whilefoo Is the SDK supposed to be used by external products such as the plugins, that would replace the their code with the createPlugin function? If so, this package is unusable by external projects as the package.json defines src/worker.ts as the main and does not define the exports, so it cannot be used outside of the kernel itself.

Otherwise if it is meant only for debug purposes it is fine, but since you added tsup and a build I suppose it is meant to be imported by other projects.

whilefoo commented 3 months ago

You're correct, it's supposed to be used by plugins. Thanks for noticing, it seems I forgot to transfer changes from PR in a separate repo

gentlementlegen commented 3 months ago

@whilefoo I testing importing it in environments where "moduleResolution": "Node" was set in tsconfig.json and it failed. From my experience it is easier to use tsup with legacyOutput set which will move the ESM generated files under esm folder, which usually fixes import issues. Worked fine in ESM mode.

I'd suggest the following configuration:

import { defineConfig } from "tsup";

export default defineConfig({
  entry: ["src/sdk/index.ts"],
  format: ["cjs", "esm"],
  outDir: "dist",
  splitting: false,
  sourcemap: false,
  clean: true,
  dts: true,
  legacyOutput: true,
});

withing package.json

  "exports": {
    ".": {
      "require": "./dist/index.js",
      "import": "./dist/esm/index.js",
      "types": "./dist/index.d.ts"
    }
  },
  "files": [
    "dist"
  ],
  "module": "dist/esm/index.js",
  "main": "dist/index.js",
  "typings": "dist/index.d.ts",

Also in the example you gave me you use Context which is not exported by the package.

whilefoo commented 3 months ago

Thanks for the help! I've never done this so I don't have much experience

gentlementlegen commented 3 months ago

Tested with the following code:

import { serve } from "@hono/node-server";
import { createPlugin, Context } from "@ubiquity-dao/ubiquibot-kernel";

type Config = { test: boolean };

type Env = { test: string };

type SupportedEvents = "issue_comment.created";

createPlugin(
  async (context: Context<Config, Env, SupportedEvents>) => {
    context.logger.info("Hello world");
    return { success: true };
  },
  { name: "", commands: {}, description: "desc", "ubiquity:listeners": [] },
).then((o) => serve(o));

Accessing http://localhost:3000/manifest.json returned

{"name":"","commands":{},"description":"desc","ubiquity:listeners":[]}

On a POST request with a wrong payload, however, the instance crashes with a 500 error Internal Server Error. I notice that if no key is provided, it defaults to an empty string: https://github.com/ubiquity/ubiquibot-kernel/blob/34adce187ac254db3b3cb2dfb52f044c7809c19b/src/sdk/server.ts#L35

Is this the intended behavior?

whilefoo commented 3 months ago

options?.ubiquibotKernelPublicKey is only intended for testing, for example if you are developing a plugin and you have your own local kernel, making tests, or debugging.

UBIQUIBOT_KERNEL_PUBLIC_KEY will be filled with a value of our official bot

gentlementlegen commented 3 months ago

@whilefoo Got it. I am fine with the code. Do you intend on changing the plugins and the template with this new package?

whilefoo commented 3 months ago

@whilefoo Got it. I am fine with the code. Do you intend on changing the plugins and the template with this new package?

I will change worker plugins, sdk for action plugins will be coming soon

0x4007 commented 3 months ago

options?.ubiquibotKernelPublicKey is only intended for testing, for example if you are developing a plugin and you have your own local kernel, making tests, or debugging.

UBIQUIBOT_KERNEL_PUBLIC_KEY will be filled with a value of our official bot

We should phase out the use of "ubiquibot" perhaps you can use KERNEL_PUBLIC_KEY instead