Allow Prefer: return=minimal for C(R)UD

[soedirgo]

What kind of change does this PR introduce?

Feature.

What is the current behavior?

Prefer: return header is set to representation by default, but this gives an error when inserting/updating/deleting on, say, a write-only table.

What is the new behavior?

Allow the user to set it to minimal by setting returnInserted/returnUpdated/returnDeleted.

Additional context

Closes #127.

return=minimal/return=representation could be a false dichotomy here, in which case it might be better to let the user set return to 'minimal' | 'representation' | ..., but this is a safe bet for now I think.

[kiwicopple]

return=minimal/return=representation could be a false dichotomy here, in which case it might be better to let the user set

I think we should implement this now as it is more in line with PostgREST. Perhaps we do returnType with default as representation? If we make it a boolean now we're stuck with it.

Small note: return_ is a bit of a strange naming convention for JS - I've seen the underscore at the start to signify "private" but it's probably best to use a descriptive snakeCase varirable name?

[soedirgo]

Perhaps we do returnType with default as representation? If we make it a boolean now we're stuck with it.

👍

return_ is a bit of a strange naming convention for JS

return_ is because it's a strict (?) keyword in JS, so I can't use it as a variable name (but I can use it as a property name). This is used e.g. in postgrest-py (from_) and postgrest-rs (in_).

[steve-chavez]

returnInserted/returnUpdated/returnDeleted

How about just returning for the three operations? It's shorter and I think it'll be easier to remember(better DX).

[kiwicopple]

@steve-chavez could you please review this one? (Bobbie has exams this week)

Is there value on adding an option 'none' which doesn't set the return value?

It's not entirely clear to me some of the options from the PostgREST docs:

• Prefer: return=representation : returns the full record
• Prefer: return=minimal: returns nothing (?)
• No Prefer: return header: (?)

[steve-chavez]

No Prefer: return header: (?)

@kiwicopple Yes, that's something that's going to be corrected for PostgREST here: PostgREST/postgrest#1656. The No Prefer is going to be called return=headers now. This option only works for POST and it adds a Location: <pk> header(for HTTP compliance) on successful insertions.

Summarizing:

• POST has 3 possible returns return=minimal/headers/representation. Defaults to return=header.
• PATCH/DELETE/PUT only have 2 return=minimal/representation. They default to return=minimal.

Is there value on adding an option 'none' which doesn't set the return value?

I think users of postgrest-js would be better served by just having a returning: true/false. On false it would use return=minimal and on true it would use return=representation as always. The Location header added by return=headers won't be of much use since they already get the PK with return=representation.

[kiwicopple]

I think it's probably better to stick close to PostgREST as possible - if I'm looking up in the PostgREST docs returning true, I think I would be a bit confused when it then says representation / minimal.

Also I think making it a bool could make ourselves victim to any future changes to PostgREST (maybe someone wants another type?)

@steve-chavez it looks like you're thinking of naming it return=headers-only rather than return=headers?

[steve-chavez]

@kiwicopple I thought we wanted DX to prime over correctness for postgrest-js. After all return=representation is not the default when doing writes, but postgrest-js includes it by default.

So if we want correctness in this case, I think it'd be better to just focus on minimal and the optional attribute should be called return instead of returning:

postgrest.from('users').insert({ username: 'bar' }, { return: 'minimal' })
// or representation(which is the default)
postgrest.from('users').insert({ username: 'bar' }, { return: 'representation' })

Also I think making it a bool could make ourselves victim to any future changes to PostgREST

The Prefer: return=representation/minimal headers are standard(RFC 7240). They're unlikely to change(they haven't in about 6 years).

it looks like you're thinking of naming it return=headers-only rather than return=headers?

Yes, that's still not implemented, it could go either way really. Perhaps we should leave adding return=headers/headers-only for another PR. The issue really is being able to use minimal. Since we have the return attribute adding it later shouldn't cause a breaking change. WDYT?

[kiwicopple]

DX to prime over correctness

Definitely the case, which is why I don't want to add a boolean 😂. Reading the docs is part of a good DX and I think if our docs differ from PostgREST then it might lead to confusion. I don't mind making changes to the implementation but this minimal/representation as a boolean seems like big departure

Perhaps we should leave adding return=headers/headers-only for another PR. Since we have the return attribute adding it later shouldn't cause a breaking change. WDYT?

yeah let's do it without the return=headers/headers-only for now. On the point of return vs returning - I think this one is driven by return being a keyword, which makes it awkard for developers - see Bobbie's implementation:

const return_ = returnInserted ? 'representation' : 'minimal'

Then finally, for making minimal the default, I actually agree with this and then make returning=representation in supabase-js, although this is probably a breaking change right? (Since we are no longer returning data)

[steve-chavez]

On the point of return vs returning - I think this one is driven by return being a keyword

Got it. Missed that one.

If we can make a breaking change here, how about changing the interface to this:

// this is the default(return=minimal)
postgrest.from('users').insert({ username: 'bar' })

// this is return=representation
postgrest.from('users').insert({ username: 'bar' }).returning()
// kinda corresponds with the generated SQL as well, users would be more aware that requires SELECT privileges

// this would be the eventual return=headers
postgrest.from('users').insert({ username: 'bar' }).headers()

This would go in line with the feedback on https://github.com/supabase/infrastructure/issues/391#issuecomment-733423540. Makes the operations more explicit, and it's easier to type.

[thorwebdev]

I'm not too deep in this topic, but I'd say it makes sense to default to what's most useful to the broadest audience, which I guess is return=representation?

Also, I like the approach of an options object as you had proposed earlier.

postgrest.from('users').insert({ username: 'bar' }, { return: 'minimal' })
// or representation(which is the default)
postgrest.from('users').insert({ username: 'bar' }, { return: 'representation' })

The options object can easily be typed and allows for easier changes/deprecation down the road 👍 Wdyt?

[steve-chavez]

The options object can easily be typed and allows for easier changes/deprecation down the road +1 Wdyt?

Agree 👍. Let's stay with the return=.. as an object instead of a method.

I'm not too deep in this topic, but I'd say it makes sense to default to what's most useful to the broadest audience, which I guess is return=representation?

Yes. So let's keep the current default. It's not necessary to cause a breaking change for now.

[kiwicopple]

Nice one team - agreed here. Let's ship it and we can #127

[github-actions]

🎉 This PR is included in version 0.22.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀