Proxy chance.js methods when custom fake-eggs methods don't exist #132
Conversation
mattpetrie
changed the title
mattpetrie
changed the title
| return Reflect.get(target, property, receiver); | ||
| } | ||
|
|
||
| return Reflect.get(chance, property, chance).bind(chance); |
There was a problem hiding this comment.
This felt a little hacky, but chance internally relies on this context to call methods, e.g. this.integer, and this was what I came up with to ensure it has the correct this instead of calling our custom methods.
For example, chance.latitude calls this.integer internally, and but we need integer to be chance.integer and not our custom integer method in this context since our custom method has a different argument signature.
There was a problem hiding this comment.
Omg this is so cool. 🤩
Just curious, if something like this was used instead, would the proxy then use our custom methods?
| return Reflect.get(chance, property, chance).bind(chance); | |
| return target[property] || chance[property]; |
| if (property in target) { | ||
| return Reflect.get(target, property, receiver); | ||
| } | ||
|
|
There was a problem hiding this comment.
If we wanted to specifically exclude any chance.js method we could also add more conditions here.
| @@ -108,6 +108,23 @@ export const createFakeEggs = ({ | |||
| word: createWordGenerator(chance), | |||
| zip: createZipGenerator(chance), | |||
| }; | |||
|
|
|||
| type FakeEggsCustomMethods = typeof customMethods; | |||
| interface FakeEggs | |||
There was a problem hiding this comment.
A little awkward to have the type definition here, but it does achieve the desired result of a return type that overrides chance's method types for each of our custom methods.
There was a problem hiding this comment.
I really like the idea of having access to all of chance's methods. But my suggestion here would be to expose them with fake.chance.someMethod(). In my opinion this would make it clearer that we are actually using chance, and I would know that I can just consult the documentation on https://chancejs.com/ for any of those methods. Also, by doing this, we could take advantage of using chance's implementation of methods with the same name if we choose too. An example of that is integer that in chance generates the full range of safe JS integers and uses named args min and max.
I understand that we might want to have the control over the implementation of some methods. @mattpetrie mentioned the example of lat and long that in fake-eggs we might want to limit to the state of California. But I would argue that whenever we decided to change an implementation of a method we should actually consider creating a more specific method like fake.california.lat and fake.california.long and keep the raw chance method available for other more general uses. This would even be better so that code that already rely on a current implementation won't break.
This is more of personal preference though.
There was a problem hiding this comment.
Oh, and as I mentioned, I really like the idea of having access to all of chance's methods.
And even though I have some personal preferences that diverge a bit from this current implementation, I don't feel strongly enough about them to block this PR.
This is an awesome initiative @mattpetrie !
|
+1 bump to push this through - just went through exactly this process with #233 and its upcoming minor release. I lean toward agreeing with @shermam 's suggestion of using |
|
Funny enough @aliceslin91 Developer Enablement has prioritized pushing this through! https://www.pivotaltracker.com/story/show/180313615 |
|
I'm excited to see this finally getting pushed through! Explicitly exposing chance.js in the way suggested was actually something I was intentionally avoiding -- my thinking was that there was some benefit to not coupling fake-eggs' interface to a specific underlying library. In other words, if we were ever to decide that we wanted to use some thing other than chance.js to power fake-eggs (and we have switched random data libraries before), having |
If in the future we decide to change libraries, we need to make sure that either all of |
|
@shermam I was going to write almost exactly the same comment! 👍🏼 |
|
Superseded by #237 |
Background
We have established use of fake-eggs as the Good Eggs best practice for fake data generation. However, fake-eggs has many known gaps in the types of data it can generate (such as geolocation coordinates, currency, etc.). Meanwhile chance.js the library that powers fake-eggs under the hood, has dozens of additional fake data generators not currently used by fake-eggs.
Currently the process when you have a need for a generator that is not currently provided by fake-eggs is to open a PR to that manually adds a generator for that specific type of data, merge the PR, and release a new version of fake-eggs. Also, frequently the added generator is a thin wrapper that just forwards arguments to chance.js.
Changes
This PR proposes using a Proxy to fall back to calling chance.js's methods whenever a custom fake-eggs method does not exist, e.g.,
fake.ssn()will callchance.ssn()since fake-eggs does not have a customssnmethod defined, butfake.integer()would still use the custom-definedintegermethod. The proxy uses the seeded instance ofchanceso seeding is preserved.Some benefits of this are: