hoeck / simple-runtypes

Small, efficient and extendable runtype library for Typescript
MIT License
114 stars 5 forks source link

Add `nonStrict` modifier for `record` runtypes #90

Closed remnantkevin closed 1 year ago

remnantkevin commented 1 year ago

Fixes:

Description

Context

The default mode of a record runtype is strict, where strictness refers to how a runtype treats excess keys in untrusted input data:

Because the record runtype is strict by default, excess keys in its input will be treated as an error:

import * as st from "simple-runtypes"

const user = st.record({
  id: st.integer(),
  name: st.string()
})

user({ id: 1, name: "Matt" })
// => { id: 1, name: "Matt" }

user({ id: 1, name: "Matt", isAdmin: true })
// => throws st.RuntypeError: invalid keys in record

st.use(user, { id: 1, name: "Matt", isAdmin: true })
// => { ok: false, error: { reason: 'invalid keys in record [...]', ... } }

simple-runtypes currently also offers a non-strict version of the record runtype, which is called the sloppyRecord runtype. Because it is non-strict, sloppyRecord ignores excess keys:

const sloppyUser = st.sloppyRecord({
  id: st.integer(),
  name: st.string()
})

sloppyUser({ id: 1, name: "Matt" })
// => { id: 1, name: "Matt" }

sloppyUser({ id: 1, name: "Matt", isAdmin: true })
// => { id: 1, name: "Matt" }

st.use(sloppyUser, { id: 1, name: "Matt", isAdmin: true })
// => { ok: true, result: { id: 1, name: "Matt" } }

nonStrict modifier

A non-strict modifier allows strict record runtypes to be changed (modified) into non-strict versions, as opposed to having a separate non-strict runtype (see sloppyRecord above). Some of the reasons why this is desirable are outlined in #68. One of these reasons is that it allows for separation between the record's definition and the way the record is parsed. The modifier approach is similar to the way the pick, omit, and partial runtypes work (they modify a record runtype in order to create a new runtype).

Note: The "sloppy" terminology has negative connotations, and it looks untidy to have the word "sloppy" across a code base. Therefore, "sloppy" has been dropped in favour of "non-strict".

record runtypes continue to be strict by default, and the nonStrict modifier is used on an existing record runtype to create a new, non-strict record runtype. For example:

const nonStrictUser = st.nonStrict(
  st.record({
    id: st.integer(),
    name: st.string()
  })
)

nonStrictUser({ id: 1, name: "Matt" })
// => { id: 1, name: "Matt" }

nonStrictUser({ id: 1, name: "Matt", isAdmin: true })
// => { id: 1, name: "Matt" }

st.use(nonStrictUser, { id: 1, name: "Matt", isAdmin: true })
// => { ok: true, result: { id: 1, name: "Matt" } }

Note: nonStrict does not modify the given runtype in-place, but returns a new runtype based on the given runtype.

Notes on capabilities and limitations

strict and nonStrict can be used in the same runtypes

const user = st.record({
  id: st.integer(),
  name: st.string(),
  company: st.nonStrict(
    st.record({
      id: st.integer(),
      name: st.string()
    })
  )
})

user({ id: 1, name: "Matt", company: { id: 1, name: "EasyJet" } })
// => { id: 1, name: "Matt", company: { id: 1, name: "EasyJet" } }

user({ id: 1, name: "Matt", company: { id: 1, name: "EasyJet", code: "EJ" } })
// => { id: 1, name: "Matt", company: { id: 1, name: "EasyJet" } }

user({ id: 1, name: "Matt", company: { id: 1, name: "EasyJet" }, isAdmin: true })
// => throws st.RuntypeError: invalid keys in record

nonStrict is not recursive

const nonStrictUser = st.nonStrict(
  st.record({
    id: st.integer(),
    name: st.string(),
    address: st.record({
      streetNumber: st.integer(),
      streetName: st.string()
    })
  })
)

nonStrictUser({ id: 1, name: "Matt", address: { streetNumber: 42, streetName: "Sesame St" } })
// => { id: 1, name: "Matt", address: { streetNumber: 42, streetName: "Sesame St" } }

nonStrictUser({ id: 1, name: "Matt", address: { streetNumber: 42, streetName: "Sesame St" } }, isAdmin: true)
// => { id: 1, name: "Matt", address: { streetNumber: 42, streetName: "Sesame St" } }

nonStrictUser({ id: 1, name: "Matt", address: { streetNumber: 42, streetName: "Sesame St", hasAlarm: true } })
// => throws st.RuntypeError: invalid keys in record

nonStrict can only be applied to records

Record runtypes include: record, pick, omit, partial, intersection (of two records).

// āœ…
// returns non-strict record runtypes 
st.nonStrict(st.record({ id: st.integer(), name: st.string() }))
st.nonStrict(st.pick(st.record({ id: st.integer(), name: st.string() }), "id"))
st.nonStrict(st.omit(st.record({ id: st.integer(), name: st.string() }), "name"))
st.nonStrict(st.partial(st.record({ id: st.integer(), name: st.string() })))
st.nonStrict(st.intersection(st.record({ name: st.string() }), st.record({ age: st.number() })))

// āŒ
// throws st.RuntypeUsageError expected a record runtype
st.nonStrict(st.string())
st.nonStrict(st.array(st.string()))
st.nonStrict(st.union(st.number(), st.string()))
// ...

Compatibility

sloppyRecord can still be used. Under the hood, a sloppyRecord just uses the nonStrict functionality.

remnantkevin commented 1 year ago

@hoeck I have updated the PR description with more details and an overview of the functionality. This is based on my understanding at the moment, but any of it can change based on discussions we have.

I will add my questions and comments about the code itself and the way nonStrict is implemented this weekend, and hopefully we can discuss from there. Look forward to hearing from you about all this šŸ˜ƒ

remnantkevin commented 1 year ago

General question about non-strictness, records, and 'combination' runtypes (union, intersection):

Similar to pick, omit, etc., nonStrict relies on the fields property to determine whether the provided runtype is a record runtype. The fields property is needed as its value is used to reconstruct a new, modified runtype from the provided runtype. However, is it the case that all the runtypes that should have fields, do have fields? Are there cases where runtypes that you would expect to have fields (i.e. they seem like they would be record runtypes) don't have fields, either because there is a good reason, or because of a mistake? (I think this might also relate to #89)

I haven't thought through all the possibilities, but I can take a union of records and/or a discriminated union as an example. (But intersection should be considered as well.)

// (A) union of records -> no `fields`
st.union(st.record({ name: st.string() }), st.record({ lastname: st.string() }))

// (B) discriminated union -> `unions`, but no `fields`
st.union(
  st.record({ type: st.literal("standard"), accessLevel: st.number() }),
  st.record({ type: st.literal("pro"), accessLevel: st.number(), admin: st.boolean() })
)

Would we expect to be able to apply pick, omit, nonStrict, etc. to these runtypes? Currently we can't, because both don't have a fields property.

Does this make sense? Does it make sense, for example, to have a union of nonStrict runtypes vs apply nonStrict to a union?

To be clear, at this point, I'm not trying to say what does and does not make sense, but rather just wanting to have the conversation.

(Also consider the various intersection scenarios, and the case in #89)

remnantkevin commented 1 year ago

Sorry for delay on this. Planning on giving it some time this weekend.

remnantkevin commented 1 year ago

TODO

hoeck commented 1 year ago

General question about non-strictness, records, and 'combination' runtypes (union, intersection):

...

I think I didn't answer this question? Or did I already?

Would we expect to be able to apply pick, omit, nonStrict, etc. to these runtypes? Currently we can't, because both don't have a fields property.

Does this make sense? Does it make sense, for example, to have a union of nonStrict runtypes vs apply nonStrict to a union? You made a good point and yes, most things that work in Typescript should work in simple-runtypes (or generally in a good runtype library) too.

Are there cases where runtypes that you would expect to have fields (i.e. they seem like they would be record runtypes) don't have fields, either because there is a good reason, or because of a mistake? (I think this might also relate to #89)

Right, not all records implement the "reflection" interface with .fields and other exposed metadata. That would be a next step to increase the utility of simple-runtypes :D.

89 was made by coworker while we were translating some untyped Ruby code into Typescript and stumbled across the need to use omit on a non-record runtype.

To be clear, at this point, I'm not trying to say what does and does not make sense, but rather just wanting to have the conversation.

Nah that's fine to suggest stuff, no need to be so defensive I am always open to discuss stuff :D and in that case it completely makes sense.

Given your specific example with unions, it can become quite tricky to exactly mimic Typescripts behavior:

https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgMpjiAJnKXkDeAUMsmAJ4AOEAXMgOQDOG2uW9JyiSjjAMhABuEADZ0QAVwC2AI2hEAvkSKhIsRCgAKUAPaFOFanXqVdHUtwi8Bwsckmz5FrFNB0ZOnSIiZFywygAggg8jMgAvGgsOHjIAD7I2jrKRAD0qWQAFsBhUFYSImBhoFz2EADuyBIgwDogZFQoABSauGDAcCLIOcgARFg5YFDAMhLtwr0AlEQByAAaEYltHSIAPMGhAHzK6Vk9eYwFRd31cPZ1ALTVtfUBM43IAJqLAPKuYOshVowANAyW1iEonomyAA

So it looks like we need to keep union type info in addition to .fields. I you like to we can discuss this in discord or in a new issue.

remnantkevin commented 1 year ago

@hoeck

README

I have updated the README: removing sloppyRecord, adding nonStrict, and various other stylistic and formatting changes. I fully understand that style and formatting are subjective, so feel free to let me know if you'd prefer non/any of the changes to be reverted. Most are an attempt at making the formatting more consistent throughout, and a few are things I noticed a long the way. I changed a few things in the Usage Examples section to try to introduce concepts before they are used, and also to separate out different examples.

Other notes:

  1. I can add more detail for nonStrict (e.g. make it 100% clear that it only works on records, and is not recursive). Not sure what info you'd prefer to have in the README vs in JSDoc comments. I could also provide more examples for nonStrict to make these things clear. Or, I could leave it for now and we can come back to that at a later stage.
  2. I ran the yarn build-readme. I haven't interacted with this script before in simple-runtypes, so please let me know if anything in the README looks wrong.

CHANGELOG

Next is adding an entry to the CHANGELOG. This would be v7.2.0, right?

Intersections

You mentioned this previously: "support non-strict in intersections (that runtype looks quite messy to me and maybe needs a little refactoring)"

I don't feel like I have the requisite knowledge yet to be able to look into this. So, just checking: will you be looking at this?

hoeck commented 1 year ago

@hoeck

README

I have updated the README: removing sloppyRecord, adding nonStrict, and various other stylistic and formatting changes. I fully understand that style and formatting are subjective, so feel free to let me know if you'd prefer non/any of the changes to be reverted. Most are an attempt at making the formatting more consistent throughout, and a few are things I noticed a long the way. I changed a few things in the Usage Examples section to try to introduce concepts before they are used, and also to separate out different examples.

:+1: that looks good thanks!

Other notes:

1. I can add more detail for nonStrict (e.g. make it 100% clear that it only works on records, and is not recursive). Not sure what info you'd prefer to have in the README vs in JSDoc comments. I could also provide more examples for nonStrict to make these things clear. Or, I could leave it for now and we can come back to that at a later stage.

Yes, please leave that for now - in a future version nonStrict should work on any type.

2. I ran the `yarn build-readme`. I haven't interacted with this script before in simple-runtypes, so please let me know if anything in the README looks wrong.

Looks good to me :+1:

CHANGELOG

Next is adding an entry to the CHANGELOG. This would be v7.2.0, right?

Right, unless we decide to remove sloppyRecord in the same release then it would be 8.0.0. Or release that together with the record->object rename to not release too many major versions?

But for now you can also use "unreleased" in the changelog. I made the habit of adding all changes immediately to the log as I tend for forget what was all done once I'm ready to release a new version.

Intersections

You mentioned this previously: "support non-strict in intersections (that runtype looks quite messy to me and maybe needs a little refactoring)"

I don't feel like I have the requisite knowledge yet to be able to look into this. So, just checking: will you be looking at this?

Yeah will do that thanks for the reminder!

remnantkevin commented 1 year ago

@hoeck

But for now you can also use "unreleased" in the changelog.

Done šŸ‘


Right, unless we decide to remove sloppyRecord in the same release then it would be 8.0.0. Or release that together with the record->object rename to not release too many major versions?

Maybe?:

I can understand the concern to not release too many major versions, but I also don't think it's worth bundling up too many changes into one release. Obviously up to you at the end of the day though šŸ˜„


Yeah will do that thanks for the reminder!

šŸ™‚

hoeck commented 1 year ago

Maybe?:

* 7.2.0

  * add nonStrict
  * remove sloppyRecord from docs

* 8.0.0

  * remove sloppyRecord
  * rename record to object

I can understand the concern to not release too many major versions, but I also don't think it's worth bundling up too many changes into one release. Obviously up to you at the end of the day though smile

Sounds like a good compromise :+1:

remnantkevin commented 1 year ago

šŸŽ‰

@hoeck Thanks for all the guidance and help on this šŸ˜„