Transition to TypeScript #93

soedirgo · 2020-09-02T09:25:22Z

@kiwicopple in hindsight, these are a lot to go through at once, so I should've gone through them one by one as they come up 🤦‍♂️. Sorry!

What kind of change does this PR introduce?

As per title. Closes #84.

Additional context

This PR introduces a rewrite of postgrest-js in TypeScript. There are some decisions I made that warrants a second opinion, so I'm listing all the important changes here:

Use Fetch polyfill in place of Superagent

Main discussion here.

Target CommonJS & ES6 for builds (no UMD, no bundlers)

The goal of targeting UMD is for easy import with <script> in browsers. Apparently, this job is already done for us: jspm provides npm packages as fully optimized ES6 modules (with source maps, code-splitting, etc.). Here's an example using their sandbox, or alternatively you can serve an HTML file with the following content:

<!doctype html>
<script type="module">
 import fetch from "https://jspm.dev/isomorphic-unfetch";

 (async () => {
     const res = await fetch('https://hacks.soedirgo.dev/postgrest/todos');
     const body = await res.json();
     document.body.innerHTML = `<pre>${JSON.stringify(body, null, 2)}</pre>`;
 })();
</script>
<body>
    <div id="container"></div>
    <canvas id="canvas"></canvas>
</body>

Use Jest for testing in place of Mocha & doctests-js

The main argument is it has snapshot testing built-in, which plays really well for data-oriented libraries like this. It allows easy testing of whole responses instead of single properties, keeping tests concise.

As for doctests-js, I really like the idea, and it's present in Rust's doc comments which I use in postgrest-rs, but I don't think it plays well with TypeScript.

Use TypeDoc for docs in the `gh-pages` branch

We previously used JSDoc, and TypeDoc is just that, but for TypeScript. It'll live in a separate branch to keep the repo clean, and automated with semantic-release.

Enforce method chaining order: CRUD (e.g. `select`), filters (e.g. `eq`), transforms (e.g. `single`)

Arguments for this can be found here.

Remove guard for using `update`/`delete` without filters & "type error" responses

This one is opiniated, but I think we should operate on the principle of least surprise, and not add "surprising" behaviors on top of PostgREST. Following PostgREST's approach, destructive operations should be mentioned in the docs (and made to stand out), but not prevented.

Standardize optional parameters

I'm taking the approach used by insert: required parameters are individual arguments, "options" live in a single object which becomes an optional last parameter. Example: insert({ name: 'Supabase', location: 'Singapore' }, { upsert: true, ... }). Affected methods:

order: ascending, nullsFirst, foreignTable (for foreign table columns)
limit: foreignTable
range: foreignTable
fts, plfts, phfts, wfts: config

Remove handling of the `%` character

PostgREST docs instructs using * to escape %. Apparently this isn't necessary? The following line works:

curl -s 'https://hacks.soedirgo.dev/postgrest/todos?or=(task.not.like.%25fin%25)' | jq

P.S. %25 is the percent-encoding for %. Need to confirm this with @steve-chavez.

On `filter` and `match`

I'm keeping these for backward compatibility, but they don't do anything beyond what filter methods already do, so I'm not sure what to do with them.

Implementation is in, all that's left to do is tests (+fixes) and docs.

kiwicopple · 2020-09-02T09:56:07Z

Amazing PR Bobbie - will take me a while to go through but I'll get through this asap to make sure you're not blocked by anything

steve-chavez · 2020-09-02T23:54:15Z

PostgREST docs instructs using * to escape %. Apparently this isn't necessary?
P.S. %25 is the percent-encoding for %.

The * was introduced to prevent errors. For example, you have a text column with numbers and you do col=like.%23-code%, that would translate to a LIKE #-code% and not a LIKE %23-code%.

But yes, it's safe to use % if you can do a encodeURI in the lib before passing the filter to PostgREST.

Remove guard for using update/delete without filters & "type error" responses
This one is opiniated, but I think we should operate on the principle of least surprise, and not add "surprising" behaviors on top of PostgREST.

Overall I agree. On the postgrest docs we recommend using pg-safeupdate to prevent whole table UPDATEs/DELETEs.

However there's some value on doing this client-side(if for some reason the pg instance doesn't have safeupdate enabled). Perhaps supabase-js can include this check?

soedirgo · 2020-09-04T04:20:10Z

But yes, it's safe to use % if you can do a encodeURI in the lib before passing the filter to PostgREST.

👍 Good to know. Thanks Steve!

Overall I agree. On the postgrest docs we recommend using pg-safeupdate to prevent whole table UPDATEs/DELETEs.

However there's some value on doing this client-side(if for some reason the pg instance doesn't have safeupdate enabled). Perhaps supabase-js can include this check?

Interesting, I didn't know pg-safeupdate was created because of destructive UPDATE/DELETE on PostgREST. I feel slightly more open to adding the prevention, though I prefer to do it here instead of supabase-js.

The solution I prefer, if we go the preventing route, is to add pg-safeupdate to supabase/postgres.

kiwicopple · 2020-09-06T03:31:38Z

Use TypeDoc for docs

Love this. Amazing.

Use Jest for testing

I originally started with this and pulled back - can't remember why, but if you have it working and you're happy then let's go ahead

As for doctests-js

Yes let's remove

jspm provides npm packages as fully optimized ES6 modules

Such a good find. Nice one

On filter and match. I'm keeping these for backward compatibility, but they don't do anything

These convenience functions - they improve DX. The filter especially when you want to build readable nested filters. eg.

let countries = await supabase
      .from('countries')
      .select('name, cities(name, districts(name))')
      .filter('cities.population', 'gt', 10000)
      .filter('cities.districts.area', 'gt', 100)

Remove handling of the % character

Yes you can either URI encode it or make it * - either is fine.

Remove guard for using update/delete

Sounds good.

I do like Steve's approach of keeping it out of postgrest-js and adding it to supabase-js. Database extensions are heavy and have to be enabled. Should we auto-enable? That goes against the principle of least surprise for database users.

I like the model of making the peripheral libraries (postgrest, auth, realtime) utilities, then sprinkling some opinions, enrichments, and best practices to supabase-js. We can add the guards in there.

More edits

Ant & I did a long session on friday with @thorwebdev. We decided that we will from now on return errors rather than throwing errors:

supabase/supabase-js#32

There's no right or wrong here, we've chose one that fits our style. This will be a breaking change, but one that we should make now since there are other breaking changes.

soedirgo · 2020-09-06T10:21:43Z

These convenience functions - they improve DX. The filter especially when you want to build readable nested filters.

Gotcha 👍.

I like the model of making the peripheral libraries (postgrest, auth, realtime) utilities, then sprinkling some opinions, enrichments, and best practices to supabase-js.

I don't think we can handle this exclusively on the supabase-js side, but I can add an option in postgrest-js's UPDATE & DELETE to require filters, and enable it by default in supabase-js. @steve-chavez is there an easy way to add a stub filter in PostgREST? Something like ...?1=1 (which didn't work).

We decided that we will from now on return errors rather than throwing errors

Will reply in the linked issue.

steve-chavez · 2020-09-08T14:32:31Z

is there an easy way to add a stub filter in PostgREST? Something like ...?1=1 (which didn't work).

Hmm.. no, that would give a parse error. What's the use case of 1=1, you want a tautology?

soedirgo · 2020-09-08T17:58:53Z

That was the plan, but on second thought that wasn't necessary.

supabase/supabase-js#32

soedirgo · 2020-09-15T14:17:42Z

It's ready! Some notes:

The response format will follow this (except missing statusCode).
Docs will look like this. It's a bit rough, unfortunately TypeDoc's default layout doesn't look great imo, but we can tweak that some other time.
Weaving a custom type is trickier than I thought, for now I'm leaving it at any.

kiwicopple · 2020-09-16T00:51:29Z

wooo! 🎉 I'll get into this today

Re: Typedoc - I will consolidate all of the Typedocs into our main docs (and add enrichments): https://supabase-spec.netlify.app/ref/gotrue/signup/

Basically I use TypeDoc -> output to JSON, then I add a complementary YAML file for enrichments (examples in different languages). The typedoc is great, but doesn't really help newbies to understand how to actually use the library. Also I would prefer a single source of truth for our docs so the community doesn't have to maintain separate docs

kiwicopple · 2020-09-16T10:48:23Z

@soedirgo can you please add this command to run on whenever the docs are built too?

 "docs:json": "typedoc --json docs/spec.json --mode modules --includeDeclarations --excludeExternals"

soedirgo · 2020-09-16T12:24:06Z

👌Updated the test runner & docs (spec.json).

kiwicopple · 2020-09-16T12:28:14Z

That worked surprisingly well. OK I'm really happy with the work you've done here - it's the perfect baseline to start implementing gotrue-js.

What are the next steps after merging in? There are breaking changes right?

soedirgo · 2020-09-16T13:23:17Z

What are the next steps after merging in? There are breaking changes right?

Yep, we'll have to squash this PR into a feat: commit so it doesn't break existing code. After that, we can slowly transition into this version (on supabase-js, infra, etc.). Especially on supabase-js where we also need to make a feat: commit for the dependency update.

One note though: as the docs is deployed using GitHub Pages, the first commit will fail, then we need to select the gh-pages branch to deploy the docs from and then I'll manually trigger another deploy (ref).

kiwicopple · 2020-09-18T08:22:07Z

OK great - let's pull the trigger on this one. Can you handle this? Let me know if you need any help

⚡ Amazing work as usual Bobbie

More info: #93

soedirgo added 4 commits August 24, 2020 00:21

refactor: initial version on TS rewrite

ff09c79

Merge remote-tracking branch 'supabase/master' into master

8d895b1

refactor: second version of TS rewrite

7cdc7e3

Implementation is in, all that's left to do is tests (+fixes) and docs.

refactor: use fetch polyfill in place of axios

faa6294

soedirgo requested review from kiwicopple and awalias September 2, 2020 09:25

soedirgo mentioned this pull request Sep 6, 2020

Major DX change: response and error handling supabase/supabase-js#32

Closed

soedirgo added 7 commits September 15, 2020 03:41

chore: remove old files

b233f3f

chore: add/update config files

40a00c4

refactor: make any the default custom type

3e47df0

refactor: unquote columns to allow embedded filter

6d9b081

refactor: shape response format

731a3b5

supabase/supabase-js#32

test: add tests

a3df671

docs: add docs

54ea30c

soedirgo force-pushed the master branch 2 times, most recently from f61429b to dd40349 Compare September 15, 2020 14:10

soedirgo marked this pull request as ready for review September 15, 2020 14:14

chore: set up GitHub Actions

be5672f

soedirgo force-pushed the master branch from dd40349 to be5672f Compare September 16, 2020 12:16

soedirgo merged commit a2efac7 into supabase:master Sep 19, 2020

soedirgo added a commit that referenced this pull request Sep 20, 2020

feat: Transition to TypeScript

965fb15

More info: #93

soedirgo added a commit that referenced this pull request Sep 20, 2020

feat: Transition to TypeScript

2960d09

More info: #93

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transition to TypeScript #93

Transition to TypeScript #93

soedirgo commented Sep 2, 2020 •

edited

Loading

kiwicopple commented Sep 2, 2020

steve-chavez commented Sep 2, 2020

soedirgo commented Sep 4, 2020 •

edited

Loading

kiwicopple commented Sep 6, 2020

soedirgo commented Sep 6, 2020

steve-chavez commented Sep 8, 2020

soedirgo commented Sep 8, 2020

soedirgo commented Sep 15, 2020 •

edited

Loading

kiwicopple commented Sep 16, 2020

kiwicopple commented Sep 16, 2020

soedirgo commented Sep 16, 2020

kiwicopple commented Sep 16, 2020

soedirgo commented Sep 16, 2020

kiwicopple commented Sep 18, 2020

Transition to TypeScript #93

Transition to TypeScript #93

Conversation

soedirgo commented Sep 2, 2020 • edited Loading

What kind of change does this PR introduce?

Additional context

Use Fetch polyfill in place of Superagent

Target CommonJS & ES6 for builds (no UMD, no bundlers)

Use Jest for testing in place of Mocha & doctests-js

Use TypeDoc for docs in the gh-pages branch

Enforce method chaining order: CRUD (e.g. select), filters (e.g. eq), transforms (e.g. single)

Remove guard for using update/delete without filters & "type error" responses

Standardize optional parameters

Remove handling of the % character

On filter and match

kiwicopple commented Sep 2, 2020

steve-chavez commented Sep 2, 2020

soedirgo commented Sep 4, 2020 • edited Loading

kiwicopple commented Sep 6, 2020

More edits

soedirgo commented Sep 6, 2020

steve-chavez commented Sep 8, 2020

soedirgo commented Sep 8, 2020

soedirgo commented Sep 15, 2020 • edited Loading

kiwicopple commented Sep 16, 2020

kiwicopple commented Sep 16, 2020

soedirgo commented Sep 16, 2020

kiwicopple commented Sep 16, 2020

soedirgo commented Sep 16, 2020

kiwicopple commented Sep 18, 2020

soedirgo commented Sep 2, 2020 •

edited

Loading

Use TypeDoc for docs in the `gh-pages` branch

Enforce method chaining order: CRUD (e.g. `select`), filters (e.g. `eq`), transforms (e.g. `single`)

Remove guard for using `update`/`delete` without filters & "type error" responses

Remove handling of the `%` character

On `filter` and `match`

soedirgo commented Sep 4, 2020 •

edited

Loading

soedirgo commented Sep 15, 2020 •

edited

Loading