fix circular reference between modules db.js and db_ops.js #1828
Conversation
|
Hi @rogeriodegoiania, it's curious that this isn't an active problem for us. Are you trying to use these files directly in your code? |
|
I use module loading "on the fly" and before version node-mongodb-native:3.1.0 we have no circular reference problem. We solve the problem by protecting the "loader" from circular references. The standard require() returns an empty object in this scenario so the Db.SYSTEM_ constants used in the db_ops.js file will return undefined. In the simulation below, using the nodejs native require(), I tried to show that Db.SYSTEM_USER_COLLECTION returns undefined but I really do not know if any errors can occur at this point. This being an expected behavior, we can cancel this pull request.
|
|
db_ops does not reference db's constants in global scope, if it tried that it'd be in trouble. As it is by the time functions in db_ops that reference db's constants are run both modules are fully defined and the constants exist. |
|
Ok, I understood! thanks for the clarifications. |
|
Personally I wouldn't mind having a separate constants module and avoiding circular dependencies (but I am not working on the Node driver hence it's up to @mbroadst and team whether to accept this PR), I just thought I could provide an example when circular dependencies can be an issue and when they can be ok. |
|
This actually is a problematic error. I'm going to reopen this for review |
| @@ -973,12 +974,4 @@ Db.prototype.getLogger = function() { | |||
| * @type {Db} | |||
| */ | |||
|
|
|||
| // Constants | |||
There was a problem hiding this comment.
By removing this, it would technically break our public API, since these values were previously exposed publicly. Can we add the following code to ensure they are still exposed?
Object.assign(Db, dbConstants);There was a problem hiding this comment.
Yes, interesting approach.
| @@ -0,0 +1,9 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
Could we rename this file just constants.js? I imagine we have others that should move here as well
There was a problem hiding this comment.
Yes, we can rename this file.
| @@ -973,12 +974,4 @@ Db.prototype.getLogger = function() { | |||
| * @type {Db} | |||
| */ | |||
|
|
|||
| // Constants | |||
| Db.SYSTEM_NAMESPACE_COLLECTION = 'system.namespaces'; | |||
|
Sorry, I found two dependencies to db.js inside db_ops.js that I had not seen. I'm testing another alternative. |
|
@rogeriodegoiania this missed the window for the v3.1.7 release, it will make it into the next one |
|
superseded by #1858 |
The db.js module when loaded depends on the db_ops.js module and when the db_ops.js module is loaded, it depends on the db.js module resulting in a circular reference.
The only need that the module db_ops.js has in relation to the module db.js is to have access to the constants Db.SYSTEM_ * (ex: Db.SYSTEM_NAMESPACE_COLLECTION)
This change created a new module to keep the constants called db_constants.js, and the circular dependency between the db.js and db_ops.js modules has been removed.