search

mobxjs / mobx-state-tree

Full-featured reactive state management without the boilerplate
https://mobx-state-tree.js.org/
MIT License
6.9k stars 640 forks source link

Linters do not recognize `fail` as a required import in library code #2138

Closed coolsoftwaretyler closed 3 months ago

coolsoftwaretyler commented 4 months ago

We've run into this problem a few times, where TypeScript thinks fail is a Jest function, so it doesn't send an error when we're missing an import.

Here's a recent PR that fixes one instance of the problem:

https://github.com/mobxjs/mobx-state-tree/pull/2137

And we've seen it before: https://github.com/mobxjs/mobx-state-tree/issues/1989

I think this is a great issue for someone who is new to the project. We need to make sure we don't have missing imports, so we should either:

  1. Tell TypeScript that fail needs to be defined (perhaps using folder paths to differentiate, since tests are separate)
  2. Rename fail to something that doesn't collide with Jest, like mstFail or something

Either way, we should be able to verify we aren't missing any more fail imports with linting.

JuanJo4 commented 3 months ago

hey @coolsoftwaretyler, I'll work on this one ✋

coolsoftwaretyler commented 3 months ago

Hi @JuanJo4, that's great! Let me know if you need a hand.

I think I'd slightly prefer the approach where we rename the function to be like, mstFail, so we don't collide with Jest.

JuanJo4 commented 3 months ago

I think I'd slightly prefer the approach where we rename the function to be like, mstFail, so we don't collide with Jest.

agreed, and I decided to use a class instead of a function class MstError extends Error, it seems more natural to me to throw an Error and not a function.

I'll push a PR soon, not sure how to write a test for this though, any suggestions?

coolsoftwaretyler commented 3 months ago

I also have a slight preference for a function here, I assume renaming the function and renaming its call sites is easier that way, but I'm happy to consider the class if it offers some other advantage.

As for a test, I would exercise the behavior - what parameters do we ask for, and what do we expect to happen? Maybe spy on whatever console method we use in the failures.