🚀 Feature: Option to get the valid fields returned?

[ErisDS]

Hi there, I'm using this module as part of a bigger validator for Ghost themes.

After I've checked that the package.json is valid, I want to use some of the fields to inform my report (name, version, description, etc) and eventually I'd like to add a few extra validations of my own which are Ghost-specific.

At the moment, I end up having to do some work twice. I can't parse the JSON and pass pre-parsed JSON to package.json-validator as it expects a string, but I also don't believe it's currently possible to get the parsed JSON back from PJV to do further work on.

I'm happy to submit a PR, but thought I'd check first, would the option to get the valid JSON back be something that would be accepted? Any thoughts or recommendations on naming or approach?

[JoshuaKGoldberg]

I can't parse the JSON and pass pre-parsed JSON to package.json-validator as it expects a string

Indeed, that would be very nice! Tracked in #66 and accepting PRs.

get the valid JSON back

Hmm. I can see a use case for consumers who pass in a string, even after #66 is in. But parsing JSON isn't too expensive -especially given that package.json files tend to not be huge- and adding new fields to the return of the parsing function makes its API a bit more complicated. Unless someone has a real strong use case for it, I'd rather avoid it.

Marking as status: in discussion to wait for someone to post that use case. Cheers!

[michaelfaith]

Now that #66 has landed, it's possible to pass the parsed object into validate. Rather than returning the valid fields as part of the returned value, what if the output included an array of invalid field names, if errors is non-empty? Then you could just filter the input object by those invalid field names to get the valid ones. Having the list of invalid fields seems like a better parallel to the errors array as part of reporting violations. Thoughts?

[JoshuaKGoldberg]

I like that.

Going even further, should we augment the output result to link field names to errors, when possible? What if you wanted to know, for example, which error(s) link to which invalid field name(s)?

[michaelfaith]

should we augment the output result to link field names to errors, when possible?

That's kind of where my head went at first too, but I was trying to think of an approach that wouldn't be breaking. I guess rather than an array of invalid field names, it could be a Record of invalid field names to array of indices in the errors array that correspond to those fields? That way the existing errors array remains untouched, and the change is additive.

[michaelfaith]

OR(!) if we're ok with breaking changes, then we could change the errors array to be an array of error objects with message and field. That would allow you to filter by field or map to array of invalid fields. I kind of like this one as a means to support: #230 (comment). We could still do it with the keys of a Record object, but it feels a bit less ergonomic imho.

[JoshuaKGoldberg]

ok with breaking changes

👍 yeah! Despite all the tomfoolery with the automation in #230 -> #243 (whoops), we're still 0.x.

array of error objects with message and field ... keys of a Record object, but it feels a bit less ergonomic imho

Hmm, what about about it feels non-ergonomic to you? I like the idea of keying by field. That way folks can quickly do checks like if (errors.bin) to see if a field has a complaint.

The only downside I can think of would be, what if we have validations that act cross-field? Then a single 1-to-many relationship wouldn't make sense. We'd have to have many-to-many.

IMO it should be either:

• If field is always singular: 1-to-many object map
• If field may sometimes be plural: array of objects with fields property

Keeping to one-field-per-report would keep things simpler IMO. So it'd be nice if we can keep to it without losing any potential reports. Can you think of a case where we'd want a multi-field report?

[michaelfaith]

Hmm, what about about it feels non-ergonomic to you? I like the idea of keying by field. That way folks can quickly do checks like if (errors.bin) to see if a field has a complaint.

In a future state, wouldn't they just use validateBin? Or the equivalent for the field they care about.

I think the purpose of the validate function in a post-granular decomp world is that people would be using it for getting all errors for the package, and optionally doing some manipulation on that, whether it be grouping it by field Object.groupBy(result.errors, ({field}) => field), which would give a map version of the data by field, or filtering (like in the case of eslint-plugin-package-json) result.errors.filter((error) => error.field !== 'bin'), or mapping it to some other form result.errors.map((error) => error.message). It feels like it's easier to get to any one of these forms if it's a denormalized array of error objects.

Keeping to one-field-per-report would keep things simpler IMO.

For sure, I don't think this proposal changes that. It's more about making it highly consumable for whatever form the end user might want to use the information.