jest-matchers#addMatchers

[aaronabramov]

No description provided.

[aaronabramov]

this will not work until #1275 is merged.
because right now jest-matchers is executed in outside environment

[cpojer]

I don't understand why we need this global object? Can it not just be a module-local variable?

[aaronabramov]

i've had a few issues with closure variables before and try to avoid it now.
a few advantages are:

1. Better semantics. because it's global it is stored on global objects
2. No weirdness when resetting the module cache and using custom require functions
3. Accessible from the outside if ever needs to be read/modified/debugged

[cpojer]

Can you explain how you are hoping to use addMatchers in Jest? I'm quite a bit against adding new globals to Jest.

I'd prefer to re-use jest.addMatchers by making it compatible with both Jasmine2 and the new matcher system. We don't use jest.addMatchers at FB, so it is a good chance to start using it. It's ok if we are breaking the API a bit, all your matchers work requires us to go to 14.0 soon anyway and this can easily be codemodded.

Regarding the global, I am also hesitant on storing it on global. Number 3 is a strict downside. I don't want anyone to modify or read this and opening it up to this will just make someone frown in five years when they have to clean up a gigantic mess at FB because some people started abusing this. I think your #2 point is valid, however I first need to see how addMatchers is being used. If it's added from the outside context in (through jest.addMatchers), the module registry will not be an issue.

Finally, if we do decide this must absolutely be global for some reason, then I would like us to use

Object.defineProperty(global, Symbol.for('$$jest-add-matchers'), {
  // these are all defaults:
  // configurable: false,
  // enumerable: false,
  // writable: false,
  value: {…}
})

so that it is unlikely people will mess with it.

[aaronabramov]

my thoughts were to use jest.addMatchers({toBeSomething: () => {...}}); yeah.

then in jest-jasmine-adapter we define a jasmine.addMatchers = (obj) => {jest.addAddmatchers(obj)});` function to make it compatible with jasmine too. (or making jasmine compatible with jest)

so no direct accessing of the global property.

I really like the idea of using a symbol though

[aaronabramov]

@cpojer how about that?
we can always do a closure var too, it's not really a big difference :)

[cpojer]

It should use Object.create(null) as value. We don't want the prototype chain as part of the matchers.

I can live with this, even though I'd prefer it not to be global but I take the module registry reset argument. I'm happy to change this later when we inject jest.addMatchers properly.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.