fix: (5.x) make access to process.versions lazy
#12584
Conversation
|
Not sure what is the workflow. Can I get a pair of eyes on this even though in draft still? I would greatly appreciate some hints on:
|
| @@ -936,4 +936,6 @@ exports.errorToPOJO = function errorToPOJO(error) { | |||
| return ret; | |||
| }; | |||
|
|
|||
| exports.nodeMajorVersion = parseInt(process.versions.node.split('.')[0], 10); | |||
| exports.nodeMajorVersion = function nodeMajorVersion() { | |||
There was a problem hiding this comment.
Maybe we make nodeMajorVersion() return null if process.versions is undefined?
There was a problem hiding this comment.
Waiting with this change until the convo below is resolved. ⬇️
| @@ -104,7 +104,7 @@ QueryCursor.prototype._read = function() { | |||
| return _this.emit('error', error); | |||
| } | |||
| // for node >= 12 the autoDestroy will emit the 'close' event | |||
| if (utils.nodeMajorVersion < 12) { | |||
| if (utils.nodeMajorVersion() < 12) { | |||
There was a problem hiding this comment.
If we make nodeMajorVersion() return null if process.versions is undefined, add a check: const nodeMajorVersion = utils.nodeMajorVersion(); if (nodeMajorVersion != null && nodeMajorVersion < 12) { ... }
There was a problem hiding this comment.
Are we sure that in the browser scenario any of the variants (entering or not entering the branch) is correct? Shouldn't it be an error if the execution path reached this point in the browser? I understand we don't to crash too much, but this check may create a false appearance that it can still work.
However, to fully assess that I would need to understand if there is any scenario where QueryCursor can work in the browser.
There was a problem hiding this comment.
@vkarpov15 What do you think about that? ⬆️
There was a problem hiding this comment.
"Shouldn't it be an error if the execution path reached this point in the browser?" <-- sure, but we can say the same thing about a lot of the codebase. I'd prefer to avoid having individual snippets of code being aware about browser vs not browser, and instead rely on the browser build to determine what should or shouldn't be in the browser build. Ideally this code wouldn't be in the browser build at all, but that's a little harder to fix.
TLDR; let's do the simple, low-risk fix of checking if nodeMajorVersion is null here.
test/browser.test.js
Outdated
| @@ -18,6 +19,10 @@ describe('browser', function() { | |||
| exec('node --eval "require(\'./lib/browser\')"', done); | |||
| }); | |||
|
|
|||
| it('require() works in an environment where process.versions is an empty object (gh-5842)', function(done) { | |||
| exec('node --eval "const process = require(\\"node:process\\"); process.versions = {}; require(\'./lib/browser\')"', done); | |||
There was a problem hiding this comment.
Make this require('process'). Older node versions don't support require('node:process')
There was a problem hiding this comment.
Sure, will do. This test was just a placeholder as it didn't properly verify the issue.
I found that Object.defineProperty(process, 'versions', { value: {} }) works for replacing the versions object. I pushed an updated version (still without the null handling described in the other comment).
There was a problem hiding this comment.
There is another issue connected to it.
If you do require('./lib.browser.js') with process.versions replaced with {} there is another piece of code that fails. It is located in one of the dependent libraries (bluebird required by mquery).
It seems to be removed by tree shaking done by webpack as it is not present in the final browser bundle.
Thus, I made the test load the bundle file (./dist/browser.umd.js) not ./lib/browser. It may lead to additional quirks when testing locally as the file must be built before running tests.
maciasello
force-pushed
the
fix/gh-11943
branch
from
9396dbb
to
c514d95
Compare
maciasello
force-pushed
the
fix/gh-11943
branch
from
c514d95
to
f81f462
Compare
|
@vkarpov15 I pushed a fix for the tests. Linter and tests should not fail anymore. Can you look at #12584 (comment) ? |
Summary
This PR (commit: 9396dbb) fixes gh-11943 in 5.x branch. It makes access to
process.versionslazy (in a function, not at module level) and thus does not crash mongoose loaded in a browser.This PR (commit: f9146a3) also contains an attempt to implement a test for this issue. Unfortunately it does not achieve the main goal. I haven't been able to mock/replace value of
process.versionsto mimic what is available in the browser. Whatever is written to it (or its properties) is lost and the original values are always returned.I am open for any suggestion on how to achieve that and can work on the PR until it is good enough. I have checked that the main change fixes the issue in our project.
Also it includes a fix (commit: c290eea) to enable running
browser.test.jsin isolation (npx mocha --exit ./test/browser.test.js). Without the change the driver is not set before being accessed.It doesn't fail when running all tests in batch, so it raises a question if driver objects set by tests do not interfere with results of other tests.
Examples
No examples needed. For the failure, just load the original
dist/browser.umd.jsinto a browser.