Skip to content

feat: Establish test extension and shims library #8

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

Merged
merged 31 commits into from
Jul 26, 2024
Merged

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Jul 16, 2024

Establishes an extension that entrains SES with eventual send (E()) for the purpose of tests, experimentation, and prototyping. This is accomplished by the addition of two new monorepo packages:

  • @ocap/extension
  • @ocap/shims

These packages are private. The @ocap prefix is merely a convenience serving to identify monorepo packages from external packages. We are not going to claim this prefix in any public registry, and any packages that we want to publish in the future will be renamed @metamask.

Various changes are made to config files in the root workspace in service of the new packages. In particular, constraints.pro has been modified, and a number of exceptions have been added. However, reviewers are encouraged to ignore this file, since Prolog constraints are deprecated in Yarn 4, and we will eventually migrate to its JavaScript constraints system.

Added code follows existing MetaMask conventions to the greatest extent possible. Both packages are exceptional in that neither of them are TypeScript libraries, and the extension of course isn't a library at all. Therefore, neither package necessarily sets a precedent for a "conventional" package in this monorepo.

@ocap/shims

This package contains various shims that we want to import as-is, without applying any transforms. This includes ses and lockdown. Moreover, some of these shims, such as eventual-send, need to be applied in a particular order to be successfully applied. To this end, the package exports endoify.mjs, which is intended to be The One Shim to Rule Them AllTM for ocap programming purposes.

Note that, while the eventual send shim is applied, eventual send itself (E()) isn't actually used anywhere yet. We will definitively use it in the immediate future, however, so we may as well add it now.

@ocap/extension

This package consists of a headless MV3 Chrome extension. See its readme for installation and usage instructions.

The extension has three entry-points:

The extension is written in TypeScript where possible, and bundled using Vite. Shims and other files are statically imported (i.e. without bundling) using vite-plugin-static-copy. Easily testable modules are tested using Vitest, Vite's Jest-inspired and ESM-compatible test runner.

This comment was marked as resolved.

This comment was marked as resolved.

@rekmarks
Copy link
Member Author

rekmarks commented Jul 17, 2024

@SocketSecurity ignore npm/[email protected] npm/[email protected]

These are well-known build tools.

@rekmarks rekmarks force-pushed the rekm/skunkworks branch 2 times, most recently from 72b690c to 5e859ef Compare July 17, 2024 10:32
@rekmarks rekmarks changed the title feat: Establish skunkworks package feat: Establish test extension and packages Jul 18, 2024
rekmarks added 7 commits July 18, 2024 13:12
- The names of all private, unpublished packages are now prefixed with
  `@ocap` to make them more distinguishable in monorepo scripts.
- Adds eventual-send shim to `@ocap/germs`
  - This was created using Agoric's `bundle-source` in the `endo` repo.
  - When they have published the latest version of `bundle-source`, we
    can simply run it on builds ourselves.
@rekmarks
Copy link
Member Author

rekmarks commented Jul 23, 2024

@SocketSecurity ignore npm/[email protected] npm/[email protected] npm/[email protected] npm/[email protected] npm/[email protected] npm/[email protected] npm/[email protected]

These are all due to widely used dev dependencies.

@rekmarks
Copy link
Member Author

@SocketSecurity ignore npm/@metamask/[email protected] npm/@metamask/[email protected]

@rekmarks rekmarks marked this pull request as ready for review July 23, 2024 20:58
@rekmarks rekmarks requested a review from a team as a code owner July 23, 2024 20:58
Copy link
Contributor

@SMotaal SMotaal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking here is that the DX can be improved in a separate PR by defining externalModules in one place at the top and using it to populate the config:

// These are the external modules that must not be modified.
const externalModules = {
  // @ocap/shims dependencies
  './endoify.mjs': '../../shims/dist/endoify.mjs',
  './eventual-send.mjs': '../../shims/dist/eventual-send.mjs',

  // SES dependencies
  './ses.mjs': '../../../node_modules/ses/dist/ses.mjs',
  './lockdown.mjs': '../../../node_modules/ses/dist/lockdown.mjs',
};

This would make it possible to:

  • Simplify config.build.rollupOptions.external using Object.keys(externalModules).
  • Simplify config.plugins[viteStaticCopy({ targets })] using Object.values(externalModules).map(…).
  • Introduce config.resolve.alias using Object.entries(externalModules).map(…) which is needed to experiment with yarn start.

This way we'd be using a single source of truth at the top of the file.

I tested locally before making this request, but want to make sure that including those changes aligns with the intent of the current PR.


Aside: For a later PR, we should discuss using bare specifiers (see Node.js's terminology) with importmap instead of ./ specifiers, at for least third-party dependencies.

Copy link
Contributor

@SMotaal SMotaal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a well-established and well-defined convention for capitalization, where:

  • acronyms (e.g. HTML) are upper-cased (e.g. document.body.setHTMLUnsafe) unless they are at the beginning of a camel-cased name (e.g. html`<html>`).
  • abbreviations (e.g. id) are title-cased (e.g. document.getElementById) unless they are at the beginning of a camel-cased name (e.g. this.id).

The convention was documented in the standards, which escapes me now, but an older reference can be found here.

I propose that we follow this convention moving forward as it is well-established and well-defined.


Aside: const URI on line 8 seems obscure and may be changed to IFRAME_URI for added clarity.

@rekmarks rekmarks requested a review from SMotaal July 25, 2024 09:37
@rekmarks
Copy link
Member Author

@SMotaal regarding your first suggestion, I tried storing the values in a map, but failed to find a satisfying key->value mapping. Instead, I moved the values to two separate arrays at the top of the file in e64d534, with some additional clarifying comments.

// Dependencies of external modules
'../../shims/dist/eventual-send.mjs',
'../../../node_modules/ses/dist/ses.mjs',
'../../../node_modules/ses/dist/lockdown.mjs',
Copy link
Contributor

@SMotaal SMotaal Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rekmarks Can you confirm if the intent is to bundle those three modules at any point?

Given the current structure I am note sure if we want to include them in both arrays to eliminate the risk of accidental duplication down the road.

No need to make the change for this PR, I can do it in the following one, but it helps to clarify now while we are here.

Thanks 🙏

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. If we decide to touch these again (not unlikely), we should probably just bundle everything in @ocap/shims and just leave ./endoify.mjs as the only thing that needs to be copied from outside of @ocap/extension itself. When we do so, we'll remove everything under "dependencies of external modules". I'm tracking this here: #9

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we may hit duplications with separate references, nothing concrete, just something I want to keep in mind for down the road.

Let's revisit this discussion if we need to.

@rekmarks rekmarks merged commit ceedae6 into main Jul 26, 2024
18 checks passed
@rekmarks rekmarks deleted the rekm/skunkworks branch July 26, 2024 16:29
@rekmarks rekmarks changed the title feat: Establish test extension and packages feat: Establish test extension and shims library Jul 26, 2024
rekmarks added a commit that referenced this pull request Aug 7, 2024
Jest's ESM support sucks
([ref](jestjs/jest#9430)) and is held up on
some interminable issues related to Node VMs
([ref](nodejs/node#37648)). I was impressed
with `vite` and `vitest` following my experience implementing #8, and
after I discovered that `vitest` has a [browser mode for unit
tests](https://vitest.dev/guide/browser/#browser-mode)—which will
ultimately allow us to test under `ses` in an actual browser
environment—I was sold.

This migrates the entire repo from `jest` to `vitest`. They work very
similarly, except [`vitest` has a number of benefits over
`jest`](https://vitest.dev/guide/comparisons.html), such as not
polluting the global namespace by default. I tried running some tests
under `ses` with lockdown in the browser, and it worked perfectly.
However, it's held up on rewriting tests that rely on JSDOM, so we're
tracking that work in #12.
SMotaal added a commit that referenced this pull request Aug 15, 2024
Resolves:
- #10 

This use `@endo/bundle-source` to bundle `eventual-send` in
`@ocap/shims` from its `node_modules` source instead of the pre-bundled
`src/eventual-send.mjs` that was introduced in #8 for expediency.

Possible concerns:

- The changes rely on
[`import.meta.resolve`](https://nodejs.org/api/esm.html#importmetaresolvespecifier)
in Node.js which is still flagged as [Stability 1.2 - Release
candidate](https://nodejs.org/api/documentation.html#stability-index).

Other notable changes:

- Bumps related `@endo/*` dependencies for `@ocap/extension` and
`@ocap/shims.
- Drops `mkdirp` from the `devDependencies` of `@ocap/shims` in favour
of using `mkdir(<path>, { recursive: true })` directly in Node.js.
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.

2 participants