Skip to content

Conversation

@SMotaal
Copy link
Contributor

@SMotaal SMotaal commented Aug 27, 2024

This optimizes the building and testing workflows for endoify using vitest.

This PR is now split out into:

  • chore(shims): Remove extraneous files and update package.json entries #39
  • refactor(shims): Optimize building and testing #40
    • Renames endoify.mjs to endoify.js in packages/{shims,extension}
      • For consistency — now all .mjs files are renamed to .js
    • Delegates bundling of endoify.js to bundleSource
      • For reliability and predictability — avoids making assumptions and resolving paths inside third-party packages
    • Optionally subs ZWJ-prefixed identifiers introduced by bundleSource with conforming CGJ alternatives
      • For conformance — makes it possible to experiment with rollup until endojs/endo#2436 lands
    • Explicitly imports ./endoify.js in packages/shims/src/*.test.ts
      • For clarity and flexibility — test files need to be explicit
    • Configures vitest to use jsdom with vmThreads for packages/shims
      • For consistency and reduced configuration overhead
  • refactor(extension): Optimize building and testing #41
    • Adds endoify.ts and endoify.test.ts to packages/extension/src
      • For clarity — sibling files importing ./endoified.js
    • Explicitly imports ./endoify.js in packages/extension/src/*.test.ts
      • For clarity and flexibility — test files need to be explicit
    • Configures vitest to use jsdom with vmThreads for packages/extension
      • For consistency and reduced configuration overhead
    • Adds guards against inlining script[src="endoify.ts"] and script[src="endoify.js"] in packages/extension/src/*.html
      • For efficacy and efficiency — parsing before vs after a failure makes no difference in speed, only in accuracy

@SMotaal SMotaal marked this pull request as ready for review August 27, 2024 22:23
@SMotaal SMotaal requested a review from a team as a code owner August 27, 2024 22:23
Comment on lines +28 to +32
if (!process.argv.includes('--without-rollup-transform')) {
source = endoScriptIdentifierTransformPlugin({
scopedRoot: path.resolve(rootDir, '../..'),
}).transform(source, specifier).code;
}
Copy link
Contributor Author

@SMotaal SMotaal Aug 27, 2024

Choose a reason for hiding this comment

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

May be better to opt into the transform than to opt out.

Suggested change
if (!process.argv.includes('--without-rollup-transform')) {
source = endoScriptIdentifierTransformPlugin({
scopedRoot: path.resolve(rootDir, '../..'),
}).transform(source, specifier).code;
}
if (process.argv.includes('--with-rollup-transform')) {
source = endoScriptIdentifierTransformPlugin({
scopedRoot: path.resolve(rootDir, '../..'),
}).transform(source, specifier).code;
}

I'm in favour of keeping it in for now. The transform is deliberate, speedy and includes opt-in options for validation and debugging for if and when they would be needed.

Some of the cons is that this is still an extra step, and that it uses regular expressions instead of AST to rewrite the bundled sources, which I am arguing against myself.

Ideally, we could upstream the replacements to @endojs/endo and avoiding needing this step all together.

Either way, the flag needs to be renamed to --without-rollup-parser-optimizations or --with-rollup-parser-optimizations or something more reflective of what is actually taking place 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I drafted endojs/endo#2436 today to see if this can be upstreamed!

@SMotaal SMotaal requested review from FUDCo and grypez August 27, 2024 22:29
Copy link
Contributor Author

@SMotaal SMotaal Aug 27, 2024

Choose a reason for hiding this comment

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

  • Correct with proper git rm and git mv before landing to avoid incorrectly aliasing file history!

    For context: From this diff, it seems that the file operations are being wrongly inferred. Instead, it seems to infer that apply-lock.mjs is being renamed to endoify.js, and that endoify.mjs is being deleted. If this lands, the history for those files will likely be misaligned with what we are explicitly intending in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, history is still not picked up sadly!

Possible solution:

  1. Remove apply-lockdown.mjs on main.
  2. Rebase against main.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is wonderful news

Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

This PR makes a lot of distinct changes, which makes it difficult to understand. It would benefit from being split into multiple, smaller PRs. Here's one possible way to slice this that would make the changes more digestible:

  • Changes specific to /shims
  • Changes specific to tests
  • Changes specific to building the extension

@SMotaal SMotaal marked this pull request as draft August 28, 2024 18:09
@SMotaal SMotaal closed this Aug 28, 2024
SMotaal added a commit that referenced this pull request Aug 28, 2024
…es (#39)

This is the first PR to land instead of #37 before #40 and #41:

- Removes extraneous files from `packages/shims/src`
  - For consistency — we will keep forgetting them otherwise
- Adds `typescript` in `devDependencies`
  - For consistency - needed since #34
SMotaal added a commit that referenced this pull request Sep 3, 2024
This is the second PR to land instead of #37 after #39 and before #41:

- Renames `endoify.mjs` to `endoify.js` in `packages/{shims,extension}`
  - For consistency — now all `.mjs` files are renamed to `.js`
- Delegates bundling of `endoify.js` to `bundleSource`
  - For reliability and predictability — avoids making assumptions and resolving paths inside third-party packages
- Optionally subs `ZWJ`-prefixed identifiers introduced by `bundleSource` with conforming `CGJ` alternatives
  - For conformance — makes it possible to [experiment with `rollup`](https://www.smotaal.io/experimental/#/experimental/modules/shim/README.md) until [endojs/endo#2436](endojs/endo#2436) rolls out.
- Explicitly imports `./endoify.js` in `packages/shims/src/*.test.ts`
  - For clarity and flexibility — test files need to be explicit
- Configures `vitest` to use `jsdom` with `vmThreads` for `packages/shims`
  - For consistency and reduced configuration overhead — thanks to @grypez's exploration putting us on this path of least resistance with `vitest`
SMotaal added a commit that referenced this pull request Sep 3, 2024
This is the third PR to land instead of #37 and after #39 and #40:

- Adds `endoify.ts` and `endoify.test.ts` to `packages/extension/src`
  - For clarity — sibling files importing `./endoified.js`
- Explicitly imports `./endoify.js` in
`packages/extension/src/*.test.ts`
  - For clarity and flexibility — test files need to be explicit
- Configures `vitest` to use `jsdom` with `vmThreads` for
`packages/extension`
  - For consistency and reduced configuration overhead
- Adds guards against inlining `script[src="endoify.ts"]` and
`script[src="endoify.js"]` in `packages/extension/src/*.html`
- For efficacy and efficiency — parsing before vs after a failure makes
no difference in speed, only in accuracy

---------

Co-authored-by: grypez <143971198+grypez@users.noreply.github.com>
@SMotaal SMotaal deleted the smotaal/test-endoify-vitest branch September 16, 2024 14:25
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.

4 participants