Open jasco opened 4 years ago
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
:memo: Please visit https://cla.developers.google.com/ to sign.
Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!
and we'll verify it.
ℹ️ Googlers: Go here for more info.
@googlebot I signed it!
From a cursory glance this looks great! I'll scrutinize a little deeper in the coming week. For better and worse, I rely solely on the unit tests rather than insights from real-world usage due to its still limited prevalence in the wild. All that to say, I may write a few new unit tests to see if there's anything this might have missed with changes in TS 4.
This leads me to conjecture that it might be time to make this a major version release. For the most part, the tool has been stable in terms of new PRs, after all it's been over a year since its launch. Some new features get added and new language constructs are handled, but by and large this feels fairly concrete. A major version release will provide the greatest benefit in changes such as this PR where the underlying TS target is upgraded.
What are your thoughts regarding this 1.0.0 major version idea?
Based on the possibility that the dependency changes might represent a breaking change for some, the major version idea makes a lot of sense. As far as a 1.0.0 release, in my very limited exploration (I just found this project yesterday), I have seen a few rough edges (possibly user errors) that might ideally be working in a 1.0 release.
The most notably problems:
I have observed a few cases where an interface member that references another interface is populated with an empty object rather than a fake. (At least a couple of cases involve references to interfaces that are import from a separate file. (Yes, they were parsed.) The secondary interface do fake properly when requested from intermock directly.
In a number of cases, I have observed arrays mock with empty objects rather than fake objects. I locally fixed the case for an array of enums, but I don't understand the Typescript code well enough to fix for the general case. I'm rather suspicious of the array's default case handling and suspect it may be the source of the problems. I looked at expanding the array kind cases to pull the data from the cached types
, but I encountered typing problems reusing the process.... functions from elsewhere in the code and it quickly got too complicated for me to know if what I was doing was correct.
An aside,
I added a issue for the first case I mentioned above, #43.
A correction regarding the second case above, it seems like I may have encountered two cases. Most of the cases I observed were actually cases where the object was declared in another file, like the first issue. The other case was when a type
alias was referenced even when declared in the same file. I added this second variant as issue #44.
@jasco @ryanmcdermott Is anyone going t merge this? Also is the issue with empty objects fixed? This library seems promising but right now the types are not generated correctly - in most cases - so it is also useless.
I could maybe help if you create an issue and describe at least roughly where the problem is. 🙂
Any update now?
This updates all the project dependencies to latest, including porting to Typescript 4.
All the tests and ci pass.
When reviewing, note in particular the destructuring immutable copy that was added in intermock.ts at line 334. The Typescript fields are now declared readonly so this somewhat complicated casting and destructuring approach, which basically should mirror what was there previously, was used to do a deep copy-update.