Skip to content

Conversation

andreivreja
Copy link
Contributor

@andreivreja andreivreja commented May 30, 2021

What kind of change does this PR introduce?

Feature

What is the current behavior?

Response is always a JSON.

What is the new behavior?

Ability to set response type to CSV. Usage:

postgrest
.from('table')
.csv()
.select()

Additional context

CSV responses can be super convenient for some use cases (#186) but also save plenty of resources used (wasted) by large response sizes (supabase/supabase-js#189)

@andreivreja
Copy link
Contributor Author

I'm thinking that a built in parser + convertor (to the same object format we're using now) would be amazing.

What's the stance on using something like fast-csv or papaparse for this? papaparse is pretty fast and wouldn't bloat the bundle size. I know you guys love using amazing open source projects, so I assume this would be fine?

@kiwicopple
Copy link
Member

Very nice PR @andreivreja 👍

@kiwicopple kiwicopple merged commit 042fa35 into supabase:master May 31, 2021
@inian
Copy link
Member

inian commented May 31, 2021

The main concern with including a csv parser is the bundle size Andrei. Our bundle is 15kb minified+gzipped now and fast-csv itself is 13.6 kb and papaparse is 6.5 kb.

@andreivreja
Copy link
Contributor Author

The main concern with including a csv parser is the bundle size Andrei. Our bundle is 15kb minified+gzipped now and fast-csv itself is 13.6 kb and papaparse is 6.5 kb.

Oh, I never actually checked the size of supabase-js, didn't expect it to be this low. Yeah, I suppose an extra 6.5kB is too high relatively speaking.

I'll mess around with papaparse these days probably and see how much is actually needed, because a good chunk of what it does isn't relevant for this.

@github-actions
Copy link

🎉 This PR is included in version 0.29.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nstrelow
Copy link

Neat that this landed.

Just some nitpicking, which I came across replicating this in dart: You could also use the response header to determine if res.headers['content-type'] == 'text/csv', instead of checking the request headers.

Seems cleaner, but should still work the same in both cases I would guess.

@andreivreja
Copy link
Contributor Author

Neat that this landed.

Just some nitpicking, which I came across replicating this in dart: You could also use the response header to determine if res.headers['content-type'] == 'text/csv', instead of checking the request headers.

Seems cleaner, but should still work the same in both cases I would guess.

Wanted to do that initially but the fact that it has charset=utf-8 in the header (and I don't really trust that it will always have it) and that I gotta trust it's always lower case made me just go for the request header instead. Sure, could've convert the case and check if the response header contains it instead of an exact match, but I felt like that's some extra resource usage.

Might just be my micro-optimization mindset, didn't actually check if it would've made any noticeable difference - probably it wouldn't haha.

And rather than checking if content-type is text/csv, could directly check if it is application/json and only parse that.

@nstrelow
Copy link

I see, then I will mirror your implementation, just to have it the same.

Consistency is probably better here ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants