Add onNoneEncoding to schema.optional

[vinassefranche]

Type

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update

Description

This adds onNoneEncoding option to schema.optiona when used with {as: "Option"}. I took inspiration from OptionFromNullishOr

Without onNoneEncoding, the prop gets removed from the containing Struct

const schema = S.Struct({
  a: S.optional(S.String, { as: "Option")
})
S.encode(schema)({ a: O.none() }) // => {}

With onNoneEncoding, the prop is kept with the value being the value of onNoneEncoding

const schema = S.Struct({
  a: S.optional(S.String, { as: "Option", onNoneEncoding: undefined })
})
S.encode(schema)({ a: O.none() }) // => { a: undefined }

Related

N/A

[changeset-bot]

🦋 Changeset detected

Latest commit: ea7a9b9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 18 packages

Name	Type
@effect/schema	Patch
@effect/cli	Patch
@effect/experimental	Patch
@effect/platform-browser	Patch
@effect/platform-bun	Patch
@effect/platform-node-shared	Patch
@effect/platform-node	Patch
@effect/platform	Patch
@effect/rpc-http	Patch
@effect/rpc	Patch
@effect/sql	Patch
@effect/sql-mssql	Patch
@effect/sql-mysql2	Patch
@effect/sql-pg	Patch
@effect/sql-sqlite-bun	Patch
@effect/sql-sqlite-node	Patch
@effect/sql-sqlite-react-native	Patch
@effect/sql-sqlite-wasm	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vinassefranche]

If this is accepted, we should update the documentation. Tell me if I should do it in this P.R.

[gcanti]

I think it would be better if onNoneEncoding had a more explicit type like () => Option.Option<X>, where X can be undefined or null | undefined, None indicates removal, while Some indicates retention (default None).

[vinassefranche]

I think it would be better if onNoneEncoding had a more explicit type like () => Option.Option<X>, where X can be undefined or null | undefined, None indicates removal, while Some indicates retention (default None).

@gcanti I did not see this comment before I answered the other.
I understand what you mean and indeed think it can be great. I'll try to update my P.R. in that way and ping you when it's done. Thanks for your input

[vinassefranche]

I think it would be better if onNoneEncoding had a more explicit type like () => Option.Option<X>, where X can be undefined or null | undefined, None indicates removal, while Some indicates retention (default None).

@gcanti why a function () => Option.Option<X> instead of simply Option.Option<X>? As it's something passed on the schema creation and it won't take the real value as an argument, I don't see the value of the function

[vinassefranche]

I think it would be better if onNoneEncoding had a more explicit type like () => Option.Option<X>, where X can be undefined or null | undefined, None indicates removal, while Some indicates retention (default None).

@gcanti why a function () => Option.Option<X> instead of simply Option.Option<X>? As it's something passed on the schema creation and it won't take the real value as an argument, I don't see the value of the function

I've implemented the version without the function so you can check if I understood well. Tell me if the global idea is good and if you think it would be better with a function.

[gcanti]

Mainly for consistency with our typical practices in Effect, although I now realize that onNoneEncoding in OptionFromNullishOr is not lazy

[gcanti]

the same applies here, it should be "?:" as before

[vinassefranche]

My bad, sorry! I'm fixing that quickly
Why not having define types that are re-used in both overloads? It feels a bit painful to have to update both every time but there might be some reasons I don't know of.

[gcanti]

yeah, now that the type is quite big, it makes even more sense to define the type only once

[vinassefranche]

Mainly for consistency with our typical practices in Effect, although I now realize that onNoneEncoding in OptionFromNullishOr is not lazy

I did the change! I was taking inspiration from OptionFromNullishOr :-)

[gcanti]

@vinassefranche I'd propose a couple of improvements WDYT? https://github.com/Effect-TS/effect/commits/2772-local/

[vinassefranche]

@vinassefranche I'd propose a couple of improvements WDYT? https://github.com/Effect-TS/effect/commits/2772-local/

@gcanti Great! I wanted to add types tests but did not know where to add them and forgot to ask.
I'd also add these tests:

// @ts-expect-error
S.optional(S.String, { as: "Option", onNoneEncoding: () => Option.some(null) })

// @ts-expect-error
S.String.pipe(S.optional({ as: "Option", onNoneEncoding: () => Option.some(null) }))

How do I integrate your commits in my branch. Can you do it on your own? Do I have to cherry-pick them?

[gcanti]

If you're okay with the changes, I think I can push to your branch. Wait, let me try...

[gcanti]

lol no, I can't do it. Please just copy the changes if you're okay with them

[mikearnaldi]

lol no, I can't do it. Please just copy the changes if you're okay with them

you should be able to, repo has the setting to allow it

[gcanti]

ok, this time it seems to have worked

[vinassefranche]

ok, this time it seems to have worked

Great! Should I push my additional type tests?