Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: isolate PostgREST features from supabase-js #13

Closed
wants to merge 1 commit into from

Conversation

soedirgo
Copy link
Member

@soedirgo soedirgo commented Aug 7, 2020

Currently, some of postgrest-js's functionalities are implemented in supabase-js, like select and order. This causes a mismatch of the implementations between the two, which caused supabase/postgrest-js#84.

Since supabase-js is made to act like postgrest-js, we should implement PostgREST-related features solely on postgrest-js and expose that from supabase-js through prototypal inheritance.

I manually tested this on a new Supabase project, but there's some problem with the test suite so I couldn't get them to pass. Will address it in a separate PR.

Currently, some of postgrest-js's functionalities are implemented in
supabase-js, like
https://github.com/supabase/supabase-js/blob/33939998516e9325e207a63a8a3f10463e89aa41/src/index.js#L137
and
https://github.com/supabase/supabase-js/blob/33939998516e9325e207a63a8a3f10463e89aa41/src/index.js#L177.
This causes a mismatch of the implementations between the two, which
caused supabase/postgrest-js#84.

Since supabase-js is made to act like postgrest-js, we should implement
PostgREST-related features solely on postgrest-js and expose that from
supabase-js through prototypal inheritance.
@soedirgo soedirgo requested a review from awalias August 7, 2020 03:36
@soedirgo soedirgo mentioned this pull request Aug 18, 2020
5 tasks
@kiwicopple
Copy link
Member

Hey @soedirgo , just want to check on this with respect to the fork you've got in your personal account.

Should I review this, or should I wait until you have finished what you're working on?

@soedirgo
Copy link
Member Author

I'm only working on postgrest-js at the moment, so it should be OK to review. I think what blocked me was some failing tests in #14 (probably an outdated Docker image).

@soedirgo
Copy link
Member Author

Forgot to mention, if this gets merged, things like filters/order/etc. before select will break once the new postgrest-js gets released, so better put it on hold I think. Old code should work unless we update postgrest-js's minor version in this repo.

@kiwicopple
Copy link
Member

OK we're going to hold off merging this until your signal Bobbie - I guess we will close this in favour of your postgrest-js changes

@soedirgo
Copy link
Member Author

👍 I also wanted to make some changes to this PR in light on the change on postgrest-js. I'll draft this for now and reopen once it's ready.

@soedirgo soedirgo marked this pull request as draft August 29, 2020 12:50
@soedirgo
Copy link
Member Author

Superseded by #50.

@soedirgo soedirgo closed this Oct 19, 2020
@soedirgo soedirgo deleted the fix/isolate-rest branch November 11, 2020 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants