fix(NODE-3528): add support for snappy 7 #2939
Conversation
| if ('kModuleError' in Snappy) { | ||
| return callback(Snappy['kModuleError']); | ||
| } | ||
| Snappy.compress(dataToBeCompressed, callback); | ||
| const snappyResult = Snappy.compress(dataToBeCompressed, callback); |
There was a problem hiding this comment.
Should we check function length here as well?
There was a problem hiding this comment.
not sure what you are asking
There was a problem hiding this comment.
We could assert the function length is what we expect it to be before assuming the last arg takes a callback, but might be brittle?
(function(){}).length === 0
(function(a){}).length === 1
(function(a, b){}).length === 2There was a problem hiding this comment.
so the danger I am seeing with assuming that the compress parameter takes a callback is that a future snappy might be updated to do something different with that second argument... it would be nice if we could determine the snappy version before calling the function
There was a problem hiding this comment.
I added some logic to check the dependency version, let me know what you think.
| "peerOptionalDependencies": { | ||
| "kerberos": "^1.1.0", | ||
| "mongodb-client-encryption": "^1.0.0", | ||
| "snappy": "^6.1.1", | ||
| "bson-ext": "^2.0.0" | ||
| }, |
There was a problem hiding this comment.
This is unused IIUC
There was a problem hiding this comment.
I'm a bit confused, isn't this what specifies the compatible versions of these dependencies? e.g., for us it's snappy 6 or 7?
There was a problem hiding this comment.
only implicitly in the sense that it happens to have some versions numbers which are kinda accurate (bson-ext is wrong). The peerOptionalDependencies was used in 3.5 by our old require_optional dependency. It's not a standard field and it isn't respected by any npm install logic, so I think dropping it, maybe in favor of a support table in our readme or docs would be better, thoughts?
There was a problem hiding this comment.
do we have an optional dependencies file that's a single point of entry for all these? because that's where I'd want to specify these; alternatively, I suppose a table in the README for an optional dependencies section would not be out of place since I guess this is how we'd tell users about these optional customizations? (let's not include bson-ext there at all though, it has worse performance than js-bson)
There was a problem hiding this comment.
I made NODE-3562 which recommends something more than a table in our README, but could be used to track that, sound good?
dariakp
added
the
Primary Review
| "peerOptionalDependencies": { | ||
| "kerberos": "^1.1.0", | ||
| "mongodb-client-encryption": "^1.0.0", | ||
| "snappy": "^6.1.1", | ||
| "bson-ext": "^2.0.0" | ||
| }, |
There was a problem hiding this comment.
I'm a bit confused, isn't this what specifies the compatible versions of these dependencies? e.g., for us it's snappy 6 or 7?
nbbeeken
force-pushed
the
NODE-3528/snappy-7
branch
from
6268da4
to
a753b25
Compare
nbbeeken
force-pushed
the
NODE-3528/snappy-7
branch
from
a753b25
to
8c6d22e
Compare
dariakp
added
Team Review
Pulling this fix out of the larger deps upgrade PR.
Reworked the bson-ext and custom fle build variants to share the custom tasks. cleans up the EVG view a bit.
Test both snappy 6 and 7.
also upgrades our connection string dep