mapbox / fusspot Goto Github PK

Looking into mapbox/mapbox-sdk-js#239, I think it would be good idea to provide astrictShapeOf which would throw error if any extra props are passed in.

Right now you can't use shape or strictShape validation on an object that has a special prototype — like, an object that is an instance of a class. I don't think there's any need for this restriction: shape validation would be just as handy with non-plain-objects.

The coordinates validator seems specific to mapbox-sdk-js.

And although "date" sounds like it might be generic enough, it's actually kind of specific to how we decide to type that for mapbox-sdk-js: anything that can be converted to a Date via new Date(x). I'm not sure that's generically useful, since it implies implementation details behind the scenes.

@kepta do you think we should keep either of these?

It would be a great feature, especially for frontend applications, if we could provide an assertion function which doesn't run any validation code when NODE_ENV=production. We can write code for this function in such a way that helps code bundler strip the validation code.

I very often find myself wanting to verify not just that a value is a string but also that that string is not empty. It's very easy to end up with a '' through automatic coercion in JS when a value is essentially undefined or null.

@kepta what do you think about adding a v.nonEmptyString?

I want to start trying to use this library frequently to check function arguments that are not necessary objects. In order to get nice clear error message, then, I need to set that apiName option. I was thinking it might be more ergonomic if we could say "If the second argument to assert is a string, we'll consider it the equivalent of apiName." — e.g. v.assert(x, 'something').

I'm also wondering if we might want to rename the apiName option property. I don't really understand the api part of that. How about description?

@kepta what do you think?

@kepta I'm thinking we should give this repo/library a less generic name. How about "formalist" or "fusspot"?

TLDR: Certain functions in node.js application could benefit from fusspot's parameter validation. This could be used as a sort of react-props for node.js which validates only when running the tests.

A lot of times a simple typo could silently sneak in and cause unexpected behaviour. In the example below, emphasise would remain falsy since foo gets the wrong parameter property.

const foo = ({ abc, xyz, tgif, emphasise }) => {
   if (emphasise) {
      return chalk.bold('I am emphasised');
   }
}

foo({ abc, xyz, tgif, emphasize }); // a simple American to British glitch

Also, if a fellow developer decides to change foo's parameter schema, she/he needs to dig into all the call sites of foo to fix it. These problems aren't anything new and can be carefully fixed with unit testing.

My proposal is that we use fusspot along with unit testing to write less test cases and catch bugs early on. A function could be wrapped around an assertion function (v.assertParams?) and the code would only validate when running tests i.e. NODE_ENV=test else it would simply be a passthrough (for performance reasons).

One of the drawbacks of this approach would be that we would need to write the parameters at two places and keep them in sync. Also, it is a non standard solution and might throw some people off.

Although typeof NaN === 'number', I'd expect that you always want to reject NaN if you're expecting a real number.

One of the goals of the library is to make it easy to integrate custom validators. Right now that feature is undocumented. Let's make sure it is in fact easy and then document the method.

@kepta I think we should revert 0.7.1 and the change for #39.

The misunderstanding there is that if a property is actually required, that should be made explicit with fusspot.required. In @kellyoung's example where f.assert(f.strictShape({name: f.string}))({}) passes, that's fine name is not required. If name were required, the schema should be changed to f.stripeShape({ name: f.required(f.string)) }).

There are valid reasons for providing longitudes that exceed +/-180, so I worry that our validator is being too strict.

Should we only enforce that latitude is +/-90, since I don't know of any reasons to exceed those limits? Or should we instead remove this validator and just check that we have an array of two numbers?

cc @mapbox/frontend-platform

Error: The following keys of value did not pass validation:

      - files: files is required.

      - settings: settings is required.

Seems like there are too many empty lines involved.

Right now if an empty object is passed to a strict shape it passes, for example:

const f = require('@mapbox/fusspot');
f.assert(f.strictShape({name: f.string}))({}) // passes

I think it should throw an error because the property name is not there.

I think every single time I use this project I also want a Date object validator.

Here's a quick implementation:

function validateDate(value) {
  if (!(value instanceof Date) || value.toString() === 'Invalid Date') {
    return 'date';
  }
};

@kepta any objections to getting this into the main library?

We should revert back to eslint and prettier, as stickler is not going forward. (Ref mapbox/stickler#7)