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

chore: implement experimental ESM stub/spy for Vite #26536

Merged
merged 51 commits into from
Apr 25, 2023
Merged

Conversation

lmiller1990
Copy link
Contributor

@lmiller1990 lmiller1990 commented Apr 19, 2023

Additional details

First release of our Vite plugin to facilitate spy/mock on ES modules.

I made this chore since

  1. It's still a POC and has known issues
  2. I want it to say below v1 until we've tested it more with some of the pilot users

Not sure the best way to handle this, but chore should only increment the patch number.

Steps to test

This is based on the PROPOSAL section from the tech brief: https://github.com/cypress-io/engineering-documentation/tree/main/tech-briefs/ct-esm-mocks#proposal. That explains what's going on here in detail.

It's complicated, don't feel you like need 100% understand it all when you review it.

Testing: Run the test suite! Or make a new Vite project that uses ES Modules and write some tests using cy.spy and cy.stub. The things in our documentation for cy.stub() and cy.spy() should work correctly with ES modules. There's a bunch of tests in this PR demonstrating it works (mostly, sans the caveats in the README).

There are some caveats, I wrote those in the README.

How has the user experience changed?

You can spy/mock ES modules (both static and dynamic imports).

PR Tasks

@lmiller1990 lmiller1990 changed the title Issue 564 esm feat: Implement experimental ESM stub/spy for Vite Apr 19, 2023
@lmiller1990 lmiller1990 changed the title feat: Implement experimental ESM stub/spy for Vite chore: implement experimental ESM stub/spy for Vite Apr 19, 2023
@lmiller1990 lmiller1990 requested a review from a team April 19, 2023 06:21
@lmiller1990
Copy link
Contributor Author

lmiller1990 commented Apr 20, 2023

I found a bug. Comment: muratkeremozcan/tour-of-heroes-react-vite-cypress-ts#103 (comment)

Something in our logic is preventing prototype based inheritance working correctly, I think... I reproduced it and added a test using the react-query library. Details below, but before that, I tried side stepping the one module (react-query) that was giving me issues:

  // Check if it is the offending module - don't proxify
  if ('useQueryErrorResetBoundary' in module) {
    return module
  }

This works - test runs fine. Instead of proxifying every module, we could do a pre-scan of the spec and only apply the proxy logic to things matching cy.stub() or cy.spy(), maybe using a regexp like cy\.(spy|stub)\((.+?). This would not work if the user wants to stub/spy a library that doesn't work with our Proxy logic - we don't really know how common those libraries are yet.

Onto my debugging:

I tracked the code down. The error is on the line this.options = this.client.defaultMutationOptions(options) , which is called here. It says defaultMutationOptions is not a function. This is supposed to be inherited from the clientQuery class, it is defined here.

I added a minimal test using class and basic inheritance - works great. Eg:

class Animal {
  voice : string = 'Hello'
  greet() {
    return this.voice
  }
}

export class Dog extends Animal {
  constructor(voice: string) {
    super()
    this.voice = voice
  }
}

new Dog('bark').greet() // works great

The issue is, I think the type of code generated when react-query is compiled. It spits out:

import _inheritsLoose from "@babel/runtime/helpers/esm/inheritsLoose";
import { Subscribable } from './subscribable';

export var MutationObserver = /*#__PURE__*/function (_Subscribable) {
  _inheritsLoose(MutationObserver, _Subscribable);

  function MutationObserver(client, options) {
    var _this;

    _this = _Subscribable.call(this) || this;
    _this.client = client;

    _this.setOptions(options);

    return _this;
  }

  var _proto = MutationObserver.prototype;

  _proto.setOptions = function setOptions(options) {
    this.options = this.client.defaultMutationOptions(options);
  };

  return MutationObserver;
}(Subscribable);

Which seems to not play nicely with our proxy code. After some debugging, the MutationObserver is supposed to have a Subscribable prototype, but after the proxy code runs, it does not have the correct prototype, it has Object.

I need to debug more, but it's not clear if 1) we've hit an actual limitation of what proxy can do or 2) if we can fix this. I suspect we should not be using Object but Reflect in general, which has more powerful introspection APIs.

lmiller1990 and others added 3 commits April 20, 2023 17:53
* Copy function prototypes across when proxying
* Add more debug logging, ensure logs are guarded by `debug` flag
* Improve perf of constructor function detection
* Fix potential nil access during spy detection
* Fix logger name
@mike-plummer mike-plummer requested a review from a team April 24, 2023 13:44
Copy link
Contributor

@marktnoonan marktnoonan left a comment

Choose a reason for hiding this comment

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

This worked well for me in testing, I've left a few minor, non blocking comments in the review and some suggestions for the readme.

return a * b
})

// Plot twist - add actually does multiplication!
Copy link
Contributor

Choose a reason for hiding this comment

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

Plot twist 😆

const About = lazy(() => import('./fixtures/exportDefaultConst'))

describe('edge cases', () => {
describe('class with constructor', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Always a bit odd to me when there's a nested describe at the top, since Cypress runs this at the bottom, after the it blocks below this. We could move this to the bottom, or wrap the tests below in their own describe block. Purely so it's easier to open the file and have the order in the command log be similar to the source order.

@@ -0,0 +1,21 @@
export class Foo {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it hard to follow Foo/Bar/Bax examples, they don't give much context for what I should expect the classes to do and I lose track of them quite quickly. Not a blocker ofc.

npm/vite-plugin-cypress-esm/README.md Outdated Show resolved Hide resolved
npm/vite-plugin-cypress-esm/README.md Outdated Show resolved Hide resolved
npm/vite-plugin-cypress-esm/README.md Outdated Show resolved Hide resolved
npm/vite-plugin-cypress-esm/README.md Outdated Show resolved Hide resolved
type: 'module',
},
// eslint-disable-next-line no-restricted-syntax
children: fs.readFileSync(MODULE_CACHE_FILEPATH, 'utf-8'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we ignore this rule here? We say Synchronous fs calls should not be used in Cypress. Use an async API instead. so might worth commenting using readFileSync because x.

@mike-plummer mike-plummer requested a review from a team April 24, 2023 18:27
Copy link
Contributor

@astone123 astone123 left a comment

Choose a reason for hiding this comment

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

Nice, hopefully we can move this into vite-dev-server once we get more feedback from users 👍🏻

@lmiller1990 lmiller1990 merged commit 466155c into develop Apr 25, 2023
@lmiller1990 lmiller1990 deleted the issue-564-esm branch April 25, 2023 02:17
tgriesser added a commit that referenced this pull request May 3, 2023
* feat/protocol:
  refactor: migrate from windi to tailwind (#26516)
  chore: update v8 generation vars so that from scratch depends implies updating the metafile (#26472)
  chore: Update Vite to 4.3.0 (#26553)
  fix: unify cdp approach to fix devtools in electron (#26573)
  dependency(deps): update dependency deps-ok to v1.4.1 🌟 (#26612)
  chore: update 12.11.0 release date (#26587)
  chore: 12.11.0 release (#26582)
  chore: implement experimental ESM stub/spy for Vite (#26536)
  chore: try triggering mouseleave on buttons to ensure that tooltips aren't showing (#26524)
  chore: add support for Angular 16 (#26052)
  chore: upgrade Vue to 3.2.47 (#26555)
  chore: Update v8 snapshot cache (#26537)
  chore: add missing utm parameters for cloud links to Debug page (#26556)
  chore: update stalebot to respect new labels and up process rate (#26552)
  fix: don't display run passing status if Cloud org is over run limit (#26533)
  chore: update vm2 to 3.9.17 (#26534)
  feat: display a limit warning on the run navigation component when there are 100 total runs (#26523)
  chore: Update v8 snapshot cache (#26476)
  chore: upgrade vm2 (#26495)
  fix: Treat Video compression 0 as false.  (#26503)
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