search

BrightonMboya / app-turso-next

A demo showing how to integrate Turso DB In a next App.
https://app-turso-next.vercel.app
MIT License
2 stars 0 forks source link

Suggestions for inclusion on turso-extended #1

Open CodingDoug opened 1 year ago

CodingDoug commented 1 year ago

  1. Instead of showing the CLI installation instructions, link to the documentation in case they ever change? We're going to apply the same change to our other repos.

  2. Put the instructions to create tables right after the shell command. That's what you'd probably want people to do right away after starting the shell.

  3. Prefer to use npm over pnpm since the former is always present.

  4. Add an instruction to install node modules before running the dev script (otherwise it will fail).

  5. When I ran the dev script, it failed with:

    ❌ Invalid environment variables:
 url: Required
 authToken: Required

error - Failed to load next.config.mjs, see more info here https://nextjs.org/docs/messages/next-config-error
Error: Invalid environment variables
    at file:///Users/doug/work/chiselstrike/codingdoug/app-turso-next/src/env/server.mjs:16:9
    at ModuleJob.run (node:internal/modules/esm/module_job:194:25)

    So, for the env vars, point out specifically which file needs to be modified to include the db url and auth token. I wasn't familiar with nextjs at all, so I had to go look up how this works. Perhaps you could instruct the user to first copy the .env.sample file to .env.local and provide the required values in it.

  6. The newest version of the CLI actually creates token with no expiration by default, so you can remove the --expiration flag if you want. But if you want to leave it on, the new proper value is never (none still works but is likely going to be deprecated).

  7. pages/index.ts shows a lint error

    Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator.eslint@typescript-eslint/no-floating-promises

    Also, I'm thinking API calls like this should also be in a react hook rather than right there in the component. What do you think? Try a web search for "react hook make api calls" for guidance from the internet.

  8. I see you copied James's responseDataAdapter. This function is no longer needed at all because the client ResultSet now allow Row objects to be indexed by both column name and number. See the documentation for example code and details. We'll be removing it from other examples as well.

  9. Update @libsql/client to the latest version 0.1.4. There might be breakage in future versions of the SDK, so we might reach out to you again to update it in order to keep this code relevant going forward.

  10. The LICENSE file should be at the root of the repo, not under src.

Optional but I think very helpful for others (fix or disable the lint rules that are causing them):

  1. src/db/turso.ts shows a lint warning:

    Unexpected default export of anonymous functioneslintimport/no-anonymous-default-export Maybe find a way to remove that by naming the export and import it by name?

  2. pages/api/turso.ts shows a bunch of lint errors about the use of any. Strict checking turns these checks on because use of any has been shown to lead to bugs due to lack of proper type information.
  3. next.config.mjs: In the first line env is unused and can be removed. This perhaps means that src/env/server.mjs is also unnecessary?
BrightonMboya commented 1 year ago

Have now resolved the above issues, looks good to go.

CodingDoug commented 1 year ago

Looking much better! Still a few more things to work out here:

  1. Please put the command to run the shell back in the instructions after creating the database. That's the easiest way to get the following SQL schema commands into play. The steps to set up should generally go 1) turso db create 2) turso db shell 3) copy and paste all the necessary SQL. 4) ".quit" to quit the shell
  2. It would be good to put all of the setup SQL into one copyable block (both the table create and the inserts) to make it easier to do the whole thing at once. (GH provides a nice copy button at the top right of each code block - assume that people will use that.)
  3. The insert statements currently have a SQL syntax error in them. I think a semicolon is missing in the first insert. Please make sure that the entire block of SQL (both create and inserts) work fine when copied and pasted directly into the turso shell.
  4. The command turso db url [DATABASE-NAME] doesn't work. You probably meantturso db show [DATABASE-NAME] --url
  5. Use the term "authentication token" instead of "secret key" to be more consistent with the documentation.
  6. There is a recommendation to create a file ".env", but that should be ".env.local", right? And they can create a template by copying the file ".env.sample"?
  7. Add one final instruction to point their browser to http://localhost:3000 to see the demo work, and explain briefly what they are seeing (when loaded, the page queries the top_destinations table and formats the results).

Don't forget to run through the entire set of steps yourself to make sure everything works the way you expect.

Optional but nice:

I just noticed that you're using images from Unsplash. They have a very permissive license, which is great. I think it would be fair to them for the README to call out in a section at the bottom that this project uses their images, and possibly even attribution for the individual artists who contributed the images in use.

BrightonMboya commented 1 year ago

Thanks for the feedback, I actually should have tested the instructions on the readme before submitting. I didnt know i had minor mistakes there. On the env variables, perhaps its a different convention, there is a .env.example file before but I have now renamed it to .env.local so that we are on the same page.

I think the readme looks good to go now