enBloc-org / kindly

GNU General Public License v3.0
12 stars 16 forks source link

"supabase" should be a dependency #216

Closed camelPhonso closed 1 week ago

camelPhonso commented 1 month ago

Expected Behaviour

On windows, the supabase package should be installed by developers as a dependency.

Current Behaviour

The supabase package is being installed by windows developers as a dev dependency. This currently causes a change in the package.json file.

mnixo commented 2 weeks ago

The current documentation instructs contributors using Windows to install supabase as a dev dependency. In theory this shouldn't be necessary, given the fact that supabase is already specified as a (non-dev) project dependency. Assuming that the project dependencies were already installed (npm i), it should be possible to run npx supabase regardless of the OS.

Since I followed the set up instructions for MacOS, I can run supabase. But because supabase is a project dependency, I can also use npx supabase:

kindly $ supabase -v                                                                                                                                                                                                                              
1.167.4
kindly $ npx supabase -v                                                                                                                                                                                                                          
1.172.2

It would be useful to validate the following on a Windows environment:

If this is validated, it would be advised to remove the instruction to install supabase as a dev dependency.

mnixo commented 2 weeks ago

On a related note, since supabase is already a project dependency, there's probably no need to install it using brew (on MacOS or Linux) either. This would ensure:

camelPhonso commented 2 weeks ago

The current documentation instructs contributors using Windows to install supabase as a dev dependency. In theory this shouldn't be necessary, given the fact that supabase is already specified as a (non-dev) project dependency. Assuming that the project dependencies were already installed (npm i), it should be possible to run npx supabase regardless of the OS.

Since I followed the set up instructions for MacOS, I can run supabase. But because supabase is a project dependency, I can also use npx supabase:

kindly $ supabase -v                                                                                                                                                                                                                              
1.167.4
kindly $ npx supabase -v                                                                                                                                                                                                                          
1.172.2

It would be useful to validate the following on a Windows environment:

  • I can create a fresh checkout of the kindly repository.
  • I can npm install its dependencies.
  • I can the npx supabase commands.

If this is validated, it would be advised to remove the instruction to install supabase as a dev dependency.

@YuraPetrovskyi @nataliiazab could one of you maybe check that this is correct and confirm here? If @mnixo is correct then we should by all means adjust our instructions.

YuraPetrovskyi commented 2 weeks ago

Hi @camelPhonso, I will try to check it

nataliiazab commented 2 weeks ago

@camelPhonso - I've tried it on my Windows machine and can confirm that it worked without npm install supabase --save-dev

YuraPetrovskyi commented 2 weeks ago

I tried to do it on my other laptop so everything was from scratch. After " npx supabase -v" I get 1.153.4 Unfortunately, after "npn supabase start" during database migration, I get an error: "Seeding data supabase\seed.sql... failed to send batch: ERROR: column "auth_code_issued_at" of relation "flow_state" does not exist (SQLSTATE 42703)"

After installing "npm install supabase --save-dev" everything worked, the migration was successful! npx supabese - v
1.176.4 I suspect that the reason is that after "npm install" the version of Supabase CLI 1.153.4 is installed. which does not allow the migration to complete successfully. What do you think about this?

P.S.: something similar was happening on my main laptop.

mnixo commented 2 weeks ago

@nataliiazab & @YuraPetrovskyi, thanks for testing this!

Unfortunately, after "npn supabase start" during database migration, I get an error: "Seeding data supabase\seed.sql... failed to send batch: ERROR: column "auth_code_issued_at" of relation "flow_state" does not exist (SQLSTATE 42703)"

If anything this should confirm that supabase is available and can be used. The error itself could be related to something else, specially when:

After installing "npm install supabase --save-dev" everything worked, the migration was successful! I suspect that the reason is that after "npm install" the version of Supabase CLI 1.153.4 is installed. which does not allow the migration to complete successfully. What do you think about this?

I agree, the (backwards) version gap could make a difference. What could be happening (that I didn't account for) is that if you've been using the --save-dev supabase version (1.176.4) until now, your supabase local instance (running in Docker) was first initialized using this version. Using the project.json version (1.153.4) to start the instance could be problematic, as this version is in fact older.

You can wipe your supabase Docker data by stopping supabase, going into Docker Desktop > Volumes > Deleting all supabase_ volumes and restarting supabase. This would erase all the changes that you've made to the database, so I won't advise you to do it.

@camelPhonso, in summary:

camelPhonso commented 2 weeks ago

Smashing - thanks @YuraPetrovskyi and @nataliiazab for testing this out.

@mnixo this is pretty sounds. Thanks for picking up on it, I feel like control over dependency versioning is a bit of a blind spot for us at the moment.

I know that there's at the moment also a script in our package.json that diffs the local schema to the Staging one and creates a new migration, so that script would need to be updated as well. So I think the issue is now:

Would you like to / have the capacity to take on this one @mnixo or should we leave it up for grabs?

mnixo commented 2 weeks ago

Would you like to / have the capacity to take on this one @mnixo or should we leave it up for grabs?

Sure. I'll run some tests and then I'll open a PR. If all goes as planned and the PR is looking good, then can handle the communication before the merge.