fix(moduleExec): pass correct object to validation

[pudgereyem]

This PR fixes #24

Changes

• moduleExec: Change so that we always pass an object to validateAndCoerceTypes
• quote: no need to default to empty object for overrides since that is done later

[gadicc]

Thanks for another PR!

Umm, on looking at the issue in #27 I think I'd prefer to go the simpler route of just defaulting to an empty object, rather than validating the default options (that never change) on every call. So instead of the objectToValidate const could we just change the

    object: queryOpts.overrides,

to

    object: queryOpts.overrides ?? {},

I'm going to add a quick test for this to make sure it works just as well. Thanks again :)

[pudgereyem]

@gadicc Ah, I see your viewpoint.

I thought it made sense to validate "ALL" query parameters, since theoretically queryOpts.overrides could change and not pass the validation? Or is this being validate elsewhere?

I saw that you solved this in another way here but it's better to account for this in moduleExec I believe. Then we can remove that.

I'll update the PR with your suggested change, but if you have time please respond to my first question.

[pudgereyem]

@gadicc this should be good to go now! And hopefully fixes the broken test that you've added here; 8017659

I think it's a really good practice to do when we found a bug btw, to add the broken test and then fix it. Nice one 💯

[gadicc]

Awesome, thanks, @pudgereyem! 🙌

And yes adding the broken test first is a great habit! I love it. I'll add a note about it on the contributing doc too.

I thought it made sense to validate "ALL" query parameters, since theoretically queryOpts.overrides could change and not pass the validation? Or is this being validate elsewhere?

I think you mean the queryOpts.defaults, right? Essentially Yahoo validates this for us on the request, so if these's a mistake in here it will get picked up during testing (although I guess I should add some URL validation into fetchDevel at some point). The defaults are set when we publish the package, they don't change at runtime and can only be overriden.

I saw that you solved this in another way here but it's better to account for this in moduleExec I believe. Then we can remove that.

You're absolutely right about this, thanks for cleaning up my code in quote too! 🙏

[pudgereyem]

I think you mean the queryOpts.defaults, right? Essentially Yahoo validates this for us on the request, so if these's a mistake in here it will get picked up during testing (although I guess I should add some URL validation into fetchDevel at some point). The defaults are set when we publish the package, they don't change at runtime and can only be overriden.

Yes, I mean queryOpts.defaults, sorry for the typo! Yeah, I see what you mean! All good 👍

[gadicc]

🎉 This PR is included in version 1.7.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀