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.

Sign up for GitHub

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

Selectively use Object.defineProperty if it is available. #303

Closed
wants to merge 1 commit into from
Closed

Selectively use Object.defineProperty if it is available. #303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions build/files.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,22 @@ function CorkedRequest(state) {
/Buffer\.prototype\.copy\.call\(src, target, offset\);/
, 'src.copy(target, offset);'
]
, maybeDefineProperty = [
headRegexp
, '$1\n' + `
/* <replacement> */
var maybeDefineProperty = Object.defineProperty;
// IE < 9
if (!maybeDefineProperty || global.document && global.document.documentMode < 9) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@ScottFreeCode you might be more familiar with IE 8 than me, can you confirm this is the correct way to detect if we are running on IE8? I need to disable it even though it is present.

Copy link

Choose a reason for hiding this comment

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

Looks right as far as I'm aware; document.documentMode is 11 on the last IE unless I fiddle with the compatibility mode, in which case it becomes 8 on compatibility mode for IE 8. Moreover, the fix did seem to work for IE7, which is even lower.

Running this bit of code (the if condition with !maybeDefineProperty replaced with !Object.defineProperty) in compatibility mode for 7, 8 and 9 got true for 7 and 8 and false for 9.

There's always brute-force try-catch-based feature sniffing to deal with the fact that IE 8 has the function but it doesn't work right; but that would probably be overkill considering this works fine as far as I can tell.

Copy link
Member Author

Choose a reason for hiding this comment

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

the problem is that it is not working as expected. The error you are seeing is due to IE 8 broken Object.defineProperty. Something in that bit of logic is wrong. I would not like to use the try-catch approach.

maybeDefineProperty = function () {};
}
/* </replacement> */
`
]
, defineProperty = [
/Object\.defineProperty/g
, 'maybeDefineProperty'
]

module.exports['_stream_duplex.js'] = [
requireReplacement
Expand All @@ -243,6 +259,8 @@ module.exports['_stream_duplex.js'] = [
, objectKeysDefine
, processNextTickImport
, processNextTickReplacement
, defineProperty
, maybeDefineProperty
]

module.exports['_stream_passthrough.js'] = [
Expand Down Expand Up @@ -277,6 +295,8 @@ module.exports['_stream_readable.js'] = [
, internalDirectory
, fixUintStuff
, addUintStuff
, defineProperty
, maybeDefineProperty
]

module.exports['_stream_transform.js'] = [
Expand Down Expand Up @@ -315,12 +335,16 @@ module.exports['_stream_writable.js'] = [
, useWriteReq
, useCorkedRequest
, addConstructors
, defineProperty
, maybeDefineProperty
]

module.exports['internal/streams/BufferList.js'] = [
safeBufferFix
, fixCopyBuffer

]

module.exports['internal/streams/destroy.js'] = [
processNextTickImport
, processNextTickReplacement
Expand Down
9 changes: 8 additions & 1 deletion lib/_stream_duplex.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,13 @@ var objectKeys = Object.keys || function (obj) {

module.exports = Duplex;

/* <replacement> */
var maybeDefineProperty = Object.defineProperty;
if (!maybeDefineProperty) {
maybeDefineProperty = function () {};
}
/* </replacement> */

/*<replacement>*/
var util = require('core-util-is');
util.inherits = require('inherits');
Expand Down Expand Up @@ -89,7 +96,7 @@ function onEndNT(self) {
self.end();
}

Object.defineProperty(Duplex.prototype, 'destroyed', {
maybeDefineProperty(Duplex.prototype, 'destroyed', {
get: function () {
if (this._readableState === undefined || this._writableState === undefined) {
return false;
Expand Down
9 changes: 8 additions & 1 deletion lib/_stream_readable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,13 @@ var processNextTick = require('process-nextick-args');

module.exports = Readable;

/* <replacement> */
var maybeDefineProperty = Object.defineProperty;
if (!maybeDefineProperty) {
maybeDefineProperty = function () {};
}
/* </replacement> */

/*<replacement>*/
var isArray = require('isarray');
/*</replacement>*/
Expand Down Expand Up @@ -186,7 +193,7 @@ function Readable(options) {
Stream.call(this);
}

Object.defineProperty(Readable.prototype, 'destroyed', {
maybeDefineProperty(Readable.prototype, 'destroyed', {
get: function () {
if (this._readableState === undefined) {
return false;
Expand Down
42 changes: 19 additions & 23 deletions lib/_stream_writable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@ var processNextTick = require('process-nextick-args');

module.exports = Writable;

/* <replacement> */
var maybeDefineProperty = Object.defineProperty;
if (!maybeDefineProperty) {
maybeDefineProperty = function () {};
}
/* </replacement> */

/* <replacement> */
function WriteReq(chunk, encoding, cb) {
this.chunk = chunk;
Expand Down Expand Up @@ -208,7 +215,7 @@ WritableState.prototype.getBuffer = function getBuffer() {

(function () {
try {
Object.defineProperty(WritableState.prototype, 'buffer', {
maybeDefineProperty(WritableState.prototype, 'buffer', {
get: internalUtil.deprecate(function () {
return this.getBuffer();
}, '_writableState.buffer is deprecated. Use _writableState.getBuffer ' + 'instead.', 'DEP0003')
Expand All @@ -221,7 +228,7 @@ WritableState.prototype.getBuffer = function getBuffer() {
var realHasInstance;
if (typeof Symbol === 'function' && Symbol.hasInstance && typeof Function.prototype[Symbol.hasInstance] === 'function') {
realHasInstance = Function.prototype[Symbol.hasInstance];
Object.defineProperty(Writable, Symbol.hasInstance, {
maybeDefineProperty(Writable, Symbol.hasInstance, {
value: function (object) {
if (realHasInstance.call(this, object)) return true;

Expand Down Expand Up @@ -408,26 +415,15 @@ function doWrite(stream, state, writev, len, chunk, encoding, cb) {

function onwriteError(stream, state, sync, er, cb) {
--state.pendingcb;
if (sync) processNextTick(afterError, stream, state, cb, er);else afterError(stream, state, cb, er);

if (sync) {
// defer the callback if we are being called synchronously
// to avoid piling up things on the stack
processNextTick(cb, er);
// this can emit finish, and it will always happen
// after error
processNextTick(finishMaybe, stream, state);
stream._writableState.errorEmitted = true;
stream.emit('error', er);
} else {
// the caller expect this to happen before if
// it is async
cb(er);
stream._writableState.errorEmitted = true;
stream.emit('error', er);
// this can emit finish, but finish must
// always follow error
finishMaybe(stream, state);
}
stream._writableState.errorEmitted = true;
stream.emit('error', er);
}

function afterError(stream, state, cb, err) {
cb(err);
finishMaybe(stream, state);
}

function onwriteStateUpdate(state) {
Expand Down Expand Up @@ -635,7 +631,7 @@ function onCorkedFinish(corkReq, state, err) {
}
}

Object.defineProperty(Writable.prototype, 'destroyed', {
maybeDefineProperty(Writable.prototype, 'destroyed', {
get: function () {
if (this._writableState === undefined) {
return false;
Expand All @@ -660,4 +656,4 @@ Writable.prototype._undestroy = destroyImpl.undestroy;
Writable.prototype._destroy = function (err, cb) {
this.end();
cb(err);
};
};