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

DOCSP-40099: Add note about client.connect() #894

Merged
merged 4 commits into from
Jun 11, 2024

Conversation

mcmorisi
Copy link
Collaborator

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-40099
Staging - https://preview-mongodbmcmorisi.gatsbyjs.io/node/DOCSP-40099-client-connect/fundamentals/connection/connect/#atlas-connection-example

Self-Review Checklist

  • Is this free of any warnings or errors in the RST?
  • Did you run a spell-check?
  • Did you run a grammar-check?

Copy link
Collaborator

@mongoKart mongoKart left a comment

Choose a reason for hiding this comment

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

Some small changes and suggestions, but nice job overall

@@ -95,6 +95,14 @@ verify that the connection is successful:
.. literalinclude:: /code-snippets/connection/stable-api-connect.js
:language: javascript

.. note::

As of v4.7, the {+driver-short+} will automatically call the ``MongoClient.connect()``
Copy link
Collaborator

Choose a reason for hiding this comment

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

S: specify driver version (as opposed to server version)

Suggested change
As of v4.7, the {+driver-short+} will automatically call the ``MongoClient.connect()``
As of {+driver-short+} v4.7, the driver will automatically call the ``MongoClient.connect()``

@@ -95,6 +95,14 @@ verify that the connection is successful:
.. literalinclude:: /code-snippets/connection/stable-api-connect.js
:language: javascript

.. note::

As of v4.7, the {+driver-short+} will automatically call the ``MongoClient.connect()``
Copy link
Collaborator

Choose a reason for hiding this comment

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

I: present tense

Suggested change
As of v4.7, the {+driver-short+} will automatically call the ``MongoClient.connect()``
As of v4.7, the {+driver-short+} automatically calls the ``MongoClient.connect()``

.. note::

As of v4.7, the {+driver-short+} will automatically call the ``MongoClient.connect()``
method when using the client to interact with your MongoDB instance, **except** when
Copy link
Collaborator

Choose a reason for hiding this comment

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

I: the prerequisite of "using the client to interact with..." isn't clear to me. what does the user have to do to get the driver to call this automatically?

.. note::

As of v4.7, the {+driver-short+} will automatically call the ``MongoClient.connect()``
method when using the client to interact with your MongoDB instance, **except** when
Copy link
Collaborator

Choose a reason for hiding this comment

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

I: there was a whole thing about a year ago about server vs. deployment vs. instance. i think the upshot was:

  • MongoDB Server = the product
  • MongoDB deployment = a running instance of MongoDB
  • MongoDB instance = not preferred
Suggested change
method when using the client to interact with your MongoDB instance, **except** when
method when using the client to interact with your MongoDB deployment, **except** when

.. note::

As of v4.7, the {+driver-short+} will automatically call the ``MongoClient.connect()``
method when using the client to interact with your MongoDB instance, **except** when
Copy link
Collaborator

Choose a reason for hiding this comment

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

S: the sentence is a little long, and the "except" clause seems pretty important. better as its own sentence?

Comment on lines 102 to 104
you use the client to call bulk operations. We recommend manually calling
``MongoClient.connect()`` if you want granular control and insight over
your application's connections.
Copy link
Collaborator

Choose a reason for hiding this comment

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

S: i think it would be useful to be more specific here; "granular control and insight" is correct, but might not be helpful to a newer user. (you could also add info about bulk writes here)

Suggested change
you use the client to call bulk operations. We recommend manually calling
``MongoClient.connect()`` if you want granular control and insight over
your application's connections.
you use the client to call bulk operations. Call the ``MongoClient.connect()`` method
explicitly if you are performing bulk operations or if you want to verify that the
connection was successful.

method when using the client to perform CRUD operations on your MongoDB deployment.
Call the ``MongoClient.connect()`` method explicitly to verify that the connection is
successful.

Copy link
Collaborator Author

@mcmorisi mcmorisi Jun 11, 2024

Choose a reason for hiding this comment

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

@nbbeeken in the #node-driver-docs Slack channel posted proof that this automatic connection still works with bulk ops.

"Call the MongoClient.connect() method explicitly if you would like to verify that the connection is successful" as per @mongoKart's suggestion gets flagged by Vale for the italicized portion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about: "...if you want to manually verify that the connection..."? As worded, it sounds like we're directing users to do it, which isn't necessary.

@mcmorisi mcmorisi requested a review from mongoKart June 11, 2024 14:47
Copy link
Collaborator

@mongoKart mongoKart left a comment

Choose a reason for hiding this comment

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

A couple of suggestions I'm happy to talk through, but LGTM otherwise!

method when using the client to perform CRUD operations on your MongoDB deployment.
Call the ``MongoClient.connect()`` method explicitly to verify that the connection is
successful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about: "...if you want to manually verify that the connection..."? As worded, it sounds like we're directing users to do it, which isn't necessary.

you use the client to call bulk operations. We recommend manually calling
``MongoClient.connect()`` if you want granular control and insight over
your application's connections.
As of {+driver-short+} v4.7, the driver automatically calls the ``MongoClient.connect()``
Copy link
Collaborator

Choose a reason for hiding this comment

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

S: this is presumably getting backported to all active docs versions, and v4.7 is in legacy. Do we need to say when this behavior started?

Suggested change
As of {+driver-short+} v4.7, the driver automatically calls the ``MongoClient.connect()``
The {+driver-short+} automatically calls the ``MongoClient.connect()``

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well spotted, happy to remove.

@mcmorisi mcmorisi merged commit 6ab9caa into mongodb:master Jun 11, 2024
1 of 2 checks passed
mcmorisi added a commit that referenced this pull request Jun 11, 2024
mcmorisi added a commit that referenced this pull request Jun 11, 2024
mcmorisi added a commit that referenced this pull request Jun 11, 2024
mcmorisi added a commit that referenced this pull request Jun 11, 2024
mcmorisi added a commit that referenced this pull request Jun 11, 2024
mcmorisi added a commit that referenced this pull request Jun 11, 2024
mcmorisi added a commit that referenced this pull request Jun 11, 2024
mcmorisi added a commit that referenced this pull request Jun 11, 2024
mcmorisi added a commit that referenced this pull request Jun 11, 2024
mcmorisi added a commit that referenced this pull request Jun 11, 2024
mcmorisi added a commit that referenced this pull request Jun 11, 2024
mcmorisi added a commit that referenced this pull request Jun 11, 2024
@mcmorisi mcmorisi deleted the DOCSP-40099-client-connect branch June 11, 2024 17:29
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