Skip to content

Make async/await the primary examples in the docs #2932

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

Merged
merged 23 commits into from
May 31, 2023

Conversation

rijkvanzanten
Copy link
Contributor

Updated all the examples in the docs to use async/await. Callbacks are mentioned as an availability in the main index, but are omitted from the examples to simplify the overall use.

I also updated the examples to use CJS instead of ESM. This is still a bit of a touchy subject in NodeJS land, so no hard feelings if we want to revert that particular commit (fcd53f9)

Closes #2855

@brianc
Copy link
Owner

brianc commented May 2, 2023

Hey sorry for missing this here - is this all ready for review? I'm a bit confused as why CF didnt' deploy a new preview version of the docs to check out live....if it's ready for final pass wanna merge master in or rebase on top so maybe the CF github action changes will pick up and deploy a preview version of the pages?

@rijkvanzanten
Copy link
Contributor Author

rijkvanzanten commented May 2, 2023

Hey sorry for missing this here - is this all ready for review? I'm a bit confused as why CF didnt' deploy a new preview version of the docs to check out live....if it's ready for final pass wanna merge master in or rebase on top so maybe the CF github action changes will pick up and deploy a preview version of the pages?

Heya! No prob. Easy to have things slip through the cracks on gh 👍🏻

This should all be ready for review. I'll try updating master from upstream to cause CF to try the auto-rebuild 👍🏻

@rijkvanzanten
Copy link
Contributor Author

@brianc Looks like you have to manually "Approve" the workflow as I'm a new contributor 🙂 Hopefully that unblocks CF 🚀

@brianc
Copy link
Owner

brianc commented May 2, 2023

nice! Turned it on. :) yeah hopefully this works... 🤞

@brianc
Copy link
Owner

brianc commented May 2, 2023

didn't work! NICE!!!! I'll just review it manually :)

@rijkvanzanten
Copy link
Contributor Author

didn't work! NICE!!!! I'll just review it manually :)

Yay for automation! 🙈 I'm assuming CF has a similar protection as Netlify does in that it doesn't auto-build previews from strangers to prevent me from DoSing your resources or bill 🙃

@@ -116,7 +116,7 @@ [email protected]
To demonstrate the issue & see if you are vunerable execute the following in node:

```js
const { Client } = require('pg')
import { Client } from 'pg'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also updated the examples to use [ESM] instead of [CJS].

This doesn’t work, see #2534.

await client.connect()

try {
const res = await client.query('SELECT $1::text as message', ['Hello world!'])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3-space indents and a semicolon appear after this point

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hooray for my VS Code settings bleeding through due to the lack of an .editorconfig 😬 I'll tweak it 👍🏻

@brianc
Copy link
Owner

brianc commented May 31, 2023

Welp the preview still isn't updating. I'll merge this and fix anything else myself I see wrong throughout the day today. Thanks for all the work and review effort involved here. ❤️

@brianc brianc merged commit 20d2c08 into brianc:master May 31, 2023
@rijkvanzanten rijkvanzanten deleted the docs branch May 31, 2023 16:26
@rijkvanzanten
Copy link
Contributor Author

Appreciate it @brianc! I had this PR bookmarked to revisit, but haven't found the time to dive deep in the ESM import point @charmander pointed out above. Happy to open another PR revising that if you already have the information available on what those imports should look like (and/or how to make those named exports work as expected in node-pg in the first place!) 🙂

@brianc
Copy link
Owner

brianc commented May 31, 2023 via email

thijs pushed a commit to thijs/node-postgres that referenced this pull request Aug 1, 2023
* Correctly capitalize GitHub

* Add note on callbacks to index

* Add note on error handling

* Update client examples to use promises

* Update pooling examples

* Fix readme link

* Update cursor docs

* Update connecting examples

* Update Queries

* Update examples in pooling

* Update trx examples

* Update SSL example

* Update example

* Use ESM instead of CJS

* Update docs/pages/apis/cursor.mdx

Co-authored-by: Charmander <[email protected]>

* Update docs/pages/apis/cursor.mdx

Co-authored-by: Charmander <[email protected]>

* Update docs/pages/apis/pool.mdx

Co-authored-by: Charmander <[email protected]>

* Update docs/pages/apis/pool.mdx

Co-authored-by: Charmander <[email protected]>

* Update docs/pages/apis/pool.mdx

Co-authored-by: Charmander <[email protected]>

* Update docs/pages/features/connecting.mdx

Co-authored-by: Charmander <[email protected]>

* Update docs/pages/features/connecting.mdx

Co-authored-by: Charmander <[email protected]>

* Update docs/pages/features/ssl.mdx

Co-authored-by: Charmander <[email protected]>

---------

Co-authored-by: Charmander <[email protected]>
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.

Make async/await the primary examples in the docs
3 participants