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

fix: add proxyHijackESM for better spyOn in browser #4701

Closed
wants to merge 7 commits into from

Conversation

samthor
Copy link
Contributor

@samthor samthor commented Dec 8, 2023

Description

Changes the way vi.spyOn works in the browser via a new experimental flag proxyHijackESM that's intended to replace slowHijackESM.
(I'm not making promises about the speed of this approach, just that it works via a Proxy)

Refixes #4264 (which wasn't "fixed", it was just disabled) - importing React now works, the React sample included now uses this 👍

The approach is vaguely:

  • Always convert static imports to dynamic ones (using HoistMocks)
  • dedups these dynamic imports inside __vitest_mocker__.wrap, so matching imports can be modified in a central location
  • instead of modifying modules that might be spied on (slowHijackESM), wraps these imports in a Proxy, which lets tinyspy work on it
    • the functions/callables of a module are further wrapped in a Proxy, so code like import { callable } from 'foo' continues to work — callable is itself now a "bound" callable Proxy, even though it looks like an orphan var

For more context, this is the start of an approach to #3046. This makes every module's import be wrapped so it can be piggybacked by vi.mock in a follow-up PR. (A complete version is here but it's not good enough for review)

Alternatives: Using iframes-per-test might make some things simpler, but I think this PR still helps — because spyOn is something we allow on any module - the HoistMocks/Proxy combination needs to run on anything a user might spy.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

Copy link

netlify bot commented Dec 8, 2023

Deploy Preview for fastidious-cascaron-4ded94 ready!

Name Link
🔨 Latest commit 6c9040f
🔍 Latest deploy log https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/657a74a61bd09f00085310f6
😎 Deploy Preview https://deploy-preview-4701--fastidious-cascaron-4ded94.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@samthor
Copy link
Contributor Author

samthor commented Dec 11, 2023

FYI I simplified the approach a bit, using the natural comparison of imported modules to match their 'proxies'. This makes the import() code less weird and relies more on whatever Vite does.

@samthor
Copy link
Contributor Author

samthor commented Jan 25, 2024

@sheremet-va Is this approach interesting to the team? Should I bother rebasing it?

I think being able to .spyOn module imports that are callables is like 50% of the way there for most people (I can .spyOn to replace a React component before it's rendered, or a function before it's called), and it's a relatively simple PR.

@sheremet-va
Copy link
Member

@sheremet-va Is this approach interesting to the team? Should I bother rebasing it?

I think being able to .spyOn module imports that are callables is like 50% of the way there for most people (I can .spyOn to replace a React component before it's rendered, or a function before it's called), and it's a relatively simple PR.

I am not sure yet how this works from your PR. Can you explain by providing code examples? How does the transformed file look in the end?

For now the priority is to finish #5036

@samthor
Copy link
Contributor Author

samthor commented Jan 30, 2024

There's actually very little transform happening. Think of it this way:

  1. Every source file has its static imports converted to dynamic ones, import { foo } from 'x' => const { foo } = await import ('x');

    • This now happens everywhere, not just where the vi.mock regexp is found. The regexp helps with hoisting mocks, but its behavior isn't really relevant to the this PR.
  2. Each import is then wrapped in a helper which converts the output to Proxy, const { foo } = await __vitest_mocker__.wrap(import('x'));

  3. The wrap function creates faux-callables which are basically ways to dereference their internal contents, which can also be set.

Here's what the final transformed source might look like:

// src.ts: the source of foo function
export function foo() { return 1; }

// consumer.ts: someone random that uses foo
const { foo } = await __vitest_mocker__.wrap(import('./src.ts'));
// outputs 1
console.debug(foo());
const oldFoo = foo;

const wholeModule = await __vitest_mocker__.wrap(import('./src.ts'));
// this is what spyOn does internally
Object.defineProperty(wholeModule, 'foo', { get() { return () => 2; } });

// outputs 2
console.debug(foo());
const newFoo = foo;

// true, because 'foo' just dereferences the current value when called
console.info(newFoo === oldFoo);

The key is that the consumer of foo always has the same reference, but now it's basically to a helper which passes through to whatever current value is stored, which can be changed by defineProperty. (It could be by literally saying wholeModule.foo = ..., but spyOn doesn't try that internally.)

This whole PR basically does the same as slowHijackESM for .spyOn, but continues to work in the React case as it doesn't mess up the default import.

@sheremet-va
Copy link
Member

Every source file has its static imports converted to dynamic ones

This would break live bindings though.

This is why the previous approach changes the imports and all references of the module use the new export. This also allows the browser to fetch static imports in a more performant way.

@samthor
Copy link
Contributor Author

samthor commented Jan 30, 2024

This would break live bindings though.

Good point. It's already a compromise made for mocking whole modules in vitest, though, and it's not called out.

to fetch static imports in a more performant way.

I appreciate this isn't a perfect solution, but it does help people test inside a browser.

If this approach isn't good for you (even behind a flag, where we can iterate on it) I can go away and think about a better solution that still fixes #4264. I suspect it might be a combination of this and slowHijackESM.

@sheremet-va
Copy link
Member

It's already a compromise made for mocking whole modules in vitest

It was a bug and it has been fixed for some time now. The bug was introduced when we migrated to more browser-friendly solution, but now it uses a code transform similar to Vite SSR.

I appreciate this isn't a perfect solution, but it does help people test inside a browser.

I believe we can provide a solution that works well out of the box, I don't feel like having it now would greatly benefit us to be honest. I think we should build on the current solution with ESM hijacking and the new mocker transform.

@samthor
Copy link
Contributor Author

samthor commented Feb 5, 2024

Fair enough, I misunderstood the project's goals re: live binding.

@samthor samthor closed this Feb 5, 2024
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.

None yet

2 participants