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

fix(db): move db constants to other file to avoid circular ref #1858

Merged
merged 2 commits into from
Oct 29, 2018

Conversation

daprahamian
Copy link
Contributor

lib/db.js referenced lib/operations/db_ops.js, which in turn
referenced lib/db.js to get some constants. Moved constants to
their own file, and used dynamic imports to get DB constructor

Fixes NODE-1692

Replaces #1828

lib/db.js referenced lib/operations/db_ops.js, which in turn
referenced lib/db.js to get some constants. Moved constants to
their own file, and used dynamic imports to get DB constructor

Fixes NODE-1692
Copy link
Contributor

@kvwalker kvwalker left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@mbroadst mbroadst left a comment

Choose a reason for hiding this comment

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

LGTM, just one request then :shipit:

@@ -0,0 +1,10 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

similar to the other PR, can we just rename this file constants.js, in the likely event we will end up with non-db related constants. Trying to contain file bloat here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think that we shouldindividually bring the constants on to the Db constructor instead of using Object.assign? If we add new constants, it will be easy to accidentally expose them.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I think that is probably best practice

@daprahamian daprahamian merged commit 239036f into master Oct 29, 2018
@daprahamian daprahamian deleted the NODE-1692/db-constants branch October 29, 2018 17:57
kiku-jw pushed a commit to kiku-jw/node-mongodb-native that referenced this pull request Feb 11, 2019
…db#1858)

* fix(db): move db constants to other file to avoid circular ref

lib/db.js referenced lib/operations/db_ops.js, which in turn
referenced lib/db.js to get some constants. Moved constants to
their own file, and used dynamic imports to get DB constructor

Fixes NODE-1692
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.

3 participants