Skip to content

Commit

Permalink
fix(serialization): normalize function stringification
Browse files Browse the repository at this point in the history
A non-default option for serialization allows for the automatically
stringifying javascript functions. Function.prototype.toString has
changed between versions of node, so we need to normalize so as to
not potentially break compatibility with earlier expectations. This
should have marginal performance effects only in the case where
users are actually using this functionality.

NODE-1499
  • Loading branch information
mbroadst committed Jun 6, 2018
1 parent 20f764c commit 1320c10
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 3 deletions.
6 changes: 4 additions & 2 deletions lib/bson/parser/calculate_size.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ var Long = require('../long').Long,
DBRef = require('../db_ref').DBRef,
Binary = require('../binary').Binary;

var normalizedFunctionString = require('./utils').normalizedFunctionString;

// To ensure that 0.4 of node works correctly
var isDate = function isDate(d) {
return typeof d === 'object' && Object.prototype.toString.call(d) === '[object Date]';
Expand Down Expand Up @@ -221,7 +223,7 @@ function calculateElement(name, value, serializeFunctions, isArray, ignoreUndefi
1 +
4 +
4 +
Buffer.byteLength(value.toString(), 'utf8') +
Buffer.byteLength(normalizedFunctionString(value), 'utf8') +
1 +
calculateObjectSize(value.scope, serializeFunctions, ignoreUndefined)
);
Expand All @@ -230,7 +232,7 @@ function calculateElement(name, value, serializeFunctions, isArray, ignoreUndefi
(name != null ? Buffer.byteLength(name, 'utf8') + 1 : 0) +
1 +
4 +
Buffer.byteLength(value.toString(), 'utf8') +
Buffer.byteLength(normalizedFunctionString(value), 'utf8') +
1
);
}
Expand Down
5 changes: 4 additions & 1 deletion lib/bson/parser/serializer.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ var writeIEEE754 = require('../float_parser').writeIEEE754,
Map = require('../map'),
Binary = require('../binary').Binary;

const normalizedFunctionString = require('./utils').normalizedFunctionString;

This comment has been minimized.

Copy link
@AramilRey

AramilRey Jun 7, 2018

This breaks compatibility with older versions of node, just changing const for var fixes the problem (as in lib/bson/parser/calculate_size.js:16)

I made a PR for this #254

This comment has been minimized.

Copy link
@mbroadst

mbroadst Jun 7, 2018

Author Member

@AramilRey 😦 sounds like its time to reintroduce 0.12 to the CI for the 1.x branch. Thanks for pointing this out, a new patch will be released shortly


// try {
// var _Buffer = Uint8Array;
// } catch (e) {
Expand Down Expand Up @@ -443,7 +445,8 @@ var serializeFunction = function(buffer, key, value, index, checkKeys, depth, is
index = index + numberOfWrittenBytes;
buffer[index++] = 0;
// Function string
var functionString = value.toString();
var functionString = normalizedFunctionString(value);

// Write the string
var size = buffer.write(functionString, index + 4, 'utf8') + 1;
// Write the size of the string to buffer
Expand Down
13 changes: 13 additions & 0 deletions lib/bson/parser/utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
'use strict';

/**
* Normalizes our expected stringified form of a function across versions of node
* @param {Function} fn The function to stringify
*/
function normalizedFunctionString(fn) {
return fn.toString().replace('function(', 'function (');

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Jun 6, 2018

I would recommend to change this to: .replace(/function *\(/, 'function ('). That way it does not matter how many spaces the function has (even though I expect it to be unified due to a linter?

This comment has been minimized.

Copy link
@mbroadst

mbroadst Jun 6, 2018

Author Member

@BridgeAR this sounds fine to me, would you be willing to PR it? I've already released the fixed packages, but we can get this in for the next release

}

module.exports = {
normalizedFunctionString

This comment has been minimized.

Copy link
@jasonout

jasonout Jun 6, 2018

@mbroadst Any reason for this to be ES6 shorthand notation and not ES5 compatible? This breaks the UglifyJS parser, causing build issues for me. Can work around it with yarn resolutions but...

This comment has been minimized.

Copy link
@nosovsh

nosovsh Jun 7, 2018

this library was working without converting to es6 before. now uglify fails on this line

This comment has been minimized.

Copy link
@mbroadst

mbroadst Jun 7, 2018

Author Member

sorry y'all, will push a fix momentarily

This comment has been minimized.

Copy link
@jasonout

jasonout Jun 7, 2018

Thanks!

This comment has been minimized.

Copy link
@mbroadst

mbroadst Jun 7, 2018

Author Member

FYI, 1.0 will be deprecated soon in favor of the 2.x branch which will be all ES6!

};

0 comments on commit 1320c10

Please sign in to comment.