Skip to content
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

SDK #64

Merged
merged 23 commits into from
Jul 30, 2024
Merged

SDK #64

merged 23 commits into from
Jul 30, 2024

Conversation

whilefoo
Copy link
Contributor

No description provided.

@whilefoo
Copy link
Contributor Author

@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
Copy link
Contributor Author

whilefoo commented Jun 22, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

I had to switch to swc because of this error aelbore/esbuild-jest#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
Copy link
Member

@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 whilefoo marked this pull request as ready for review July 16, 2024 16:29
@whilefoo
Copy link
Contributor Author

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

jest.config.js Outdated Show resolved Hide resolved
src/sdk/context.ts Outdated Show resolved Hide resolved
src/sdk/server.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@gentlementlegen
Copy link
Member

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

src/sdk/server.ts Outdated Show resolved Hide resolved
src/sdk/signature.ts Show resolved Hide resolved
eslint.config.mjs Outdated Show resolved Hide resolved
@gentlementlegen
Copy link
Member

@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
Copy link
Contributor Author

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

tsup.config.js Outdated Show resolved Hide resolved
tsup.config.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@gentlementlegen
Copy link
Member

gentlementlegen commented Jul 28, 2024

@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
Copy link
Contributor Author

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

@gentlementlegen
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

@whilefoo
Copy link
Contributor Author

@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
Copy link
Collaborator

0x4007 commented Jul 30, 2024

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

@whilefoo whilefoo merged commit 3257410 into development Jul 30, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants