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.

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

Add exports.types #32

Merged
merged 67 commits into from
May 1, 2019
Merged

Add exports.types #32

merged 67 commits into from
May 1, 2019

Conversation

lukechilds
Copy link
Member

@lukechilds lukechilds commented Apr 23, 2019

This is required for browserify/commonjs-assert#44

Methods implemented (will check these off as I go):

  • isAnyArrayBuffer
  • isArgumentsObject
  • isArrayBuffer
  • isArrayBufferView
  • isAsyncFunction
  • isBigInt64Array
  • isBigIntObject
  • isBigUint64Array
  • isBooleanObject
  • isBoxedPrimitive
  • isDataView
  • isDate
  • isExternal (non-enumerable property that throws)
  • isFloat32Array
  • isFloat64Array
  • isGeneratorFunction
  • isGeneratorObject
  • isInt16Array
  • isInt32Array
  • isInt8Array
  • isMap
  • isMapIterator
  • isModuleNamespaceObject (non-enumerable property that throws)
  • isNativeError (aliased to isError)
  • isNumberObject
  • isPromise
  • isProxy (non-enumerable property that throws)
  • isRegExp
  • isSet
  • isSetIterator
  • isSharedArrayBuffer
  • isStringObject
  • isSymbolObject
  • isTypedArray
  • isUint16Array
  • isUint32Array
  • isUint8Array
  • isUint8ClampedArray
  • isWeakMap
  • isWeakSet
  • isWebAssemblyCompiledModule

// CC @goto-bus-stop @BridgeAR

@lukechilds lukechilds changed the title Add exports.types [WIP] Add exports.types Apr 23, 2019
util.js Show resolved Hide resolved
@lukechilds
Copy link
Member Author

Ahh, of course, browser testing is disabled in Travis for PRs as the encrypted ENV vars aren't accessible.

I'll setup browser testing locally.

@lukechilds
Copy link
Member Author

Hmmn, I get 406 Not Acceptable when triyng to sign up for Sauce Labs.
https://signup.saucelabs.com/signup/trial

So I can't test on all browsers but testing in Chrome locally for now.

Added an npm script for local browser testing to make this a bit simpler and more obvious for other devs: #33

@lukechilds
Copy link
Member Author

lukechilds commented Apr 23, 2019

@goto-bus-stop any chance either I can get invited to this repo or we can move it to @browserify so I can work in a branch in the main repo and use Sauce Labs for browser testing?

@lukechilds
Copy link
Member Author

lukechilds commented Apr 23, 2019

@BridgeAR what's the best way to check for primitive wrapper objects?

Using isNumberObject as an example:

> num = 1
1
> numObject = new Number(1)
[Number: 1]
> num instanceof Number
false
> numObject instanceof Number
true
> !Number.isNaN(numObject) && numObject !== Number.prototype.valueOf.call(numObject)
true
> !Number.isNaN(num) && num !== Number.prototype.valueOf.call(num)
false

I would use instanceof but I noticed this comment in the source:

// NOTE: These type checking functions intentionally don't use `instanceof`
// because it is fragile and can be easily faked with `Object.create()`.

https://github.com/defunctzombie/node-util/blob/4ca6fa62bdc1985f831a71a94f4b41b236b52a86/util.js#L458-L459

will using Number.prototype.valueOf.call resolve the issues mentioned in the comment?

@vweevers
Copy link

What's the intended browser support? For example, Symbol is not supported in IE.

@BridgeAR
Copy link
Member

@lukechilds you could use Number.prototype.valueOf.call(numberObj). The problem is that you have to wrap it in a try catch, since this can throw.

You could use the code like in https://github.com/ljharb/is-number-object/blob/master/index.js. Symbols and BigInt are not available in all Browsers and they partially require more special handling see e.g., https://github.com/ljharb/is-symbol/blob/master/index.js. Note that the primitive value returns true in these modules. That is not ideal but otherwise they are the "safest" check I know for boxed primitives.

@lukechilds
Copy link
Member Author

lukechilds commented Apr 23, 2019

@vweevers

What's the intended browser support?

Ideally the same as it is now, I assume just the browsers in here: https://github.com/defunctzombie/node-util/blob/master/.airtap.yml

If the type isn't supported by a browser we can just always return false.

@BridgeAR

Note that the primitive value returns true in these modules. That is not ideal but otherwise they are the "safest" check I know for boxed primitives.

Isn't that a pretty big problem though? All the isFooObject functions are used inside isBoxedPrimitive. We don't want isBoxedPrimitive returning true for plain primitive values.

What's wrong with just using:

function isNumberObject(value) {
  try {
    return value === Number.prototype.valueOf.call(value);
  } catch(e) {
    return false;
  }
}

Is that not safe?

@BridgeAR
Copy link
Member

@lukechilds

Isn't that a pretty big problem though?

Sorry, I did not express myself properly: I would have used used these modules directly if they would not match the primitive as well. So yes, that's a problem and you have to manually write the functions.

[Some code...] Is that not safe?

It is possible to manipulate the global Number (and other globals) by replacing the valueOf function. So it should at least be cached on import. There is no way around this AFAIK (in Node.js we use a C++ function because of that).

The code should look like:

function isNumberObject(value) {
  if (typeof value !== 'object') {
    return false;
  }
  try {
    Number.prototype.valueOf.call(value);
    return true;
  } catch(e) {
    return false;
  }
}

@lukechilds
Copy link
Member Author

function isMapToString(value) {
  return ObjectToString(value) === '[object Map]';
}
isMapToString.working = Map && isMapToString(new Map());

function isMap(value) {
  return (
    !isMapToString.working &&
    Map
  )
  ? value instanceof Map
  : isMapToString(value);
}
exports.isMap = isMap;

Will only ever fall back to instanceof when Map is available but ObjectToString(new Map()) !== '[object Map]'.

@lukechilds
Copy link
Member Author

lukechilds commented Apr 29, 2019

Ok, it was a lot more work than I expected to get everything working in IE10/11, but they're passing all tests now.

Also, I've been testing by running the airtap test server locally, exposing it remotely through an ngrok tunnel, and then running that in virtualised browser instances on Browserling. For some reason IE9 on Browserling won't connect to my ngrok tunnel so I can't test it.

@goto-bus-stop are you able to run the browser tests again?

@lukechilds
Copy link
Member Author

Ok, I managed to test in IE9, it works 🎉

Screen Shot 2019-04-29 at 3 46 55 pm

@goto-bus-stop are you able to review this and let me know if it needs any changes?

I'm trying to get the assert PR (browserify/commonjs-assert#44) finished this week and I can't start browser testing until this PR is merged and published.

@goto-bus-stop
Copy link
Member

goto-bus-stop commented Apr 29, 2019

sick! ci: https://travis-ci.org/defunctzombie/node-util/builds/525870897 (together with #34)

(fwiw if you have lots of free disk space there are free VMs with old IE at https://developer.microsoft.com/en-us/microsoft-edge/tools/vms/)

@goto-bus-stop
Copy link
Member

goto-bus-stop commented Apr 29, 2019

Node 0.12 is failing because of Buffer.from, you can use safe-buffer to make it work everywhere.

Node 0.10 is failing because of a new DataView() call, I guess the second argument was not optional in the past? new DataView(new ArrayBuffer(), 0) or so

Firefox is failing on the Symbol.toStringTag tests … not sure about those

@lukechilds
Copy link
Member Author

Thanks, looking into Firefox issues on Browserling. Didn't expect that, running Firefox 66 locally and all tests pass.

@lukechilds
Copy link
Member Author

lukechilds commented Apr 29, 2019

Ok, how important are those failing Firefox tests? 😆

The only issue it's causing is that people can create fake TypedArray like objects that will pass assertions:

const fakeUint8Array = {[Symbol.toStringTag]: 'Uint8Array'};
util.types.isUint8Array(fakeUint8Array);
// true

Although this isn't great, it's probably not that likely to happen in the wild. This also only happens in outdated versions of Firefox. The latest version will correctly handle a fake typed array. And Firefox is auto-updating so most people should be running an up to date version.

@goto-bus-stop
Copy link
Member

yeah I'm fine with leaving that "broken". If a solution is found later we can do a bugfix release 🤷‍♀️

@lukechilds
Copy link
Member Author

Awesome!

Ok, those Node.js issues should be fixed.

@lukechilds
Copy link
Member Author

@goto-bus-stop Is this ok to merge now?

@lukechilds
Copy link
Member Author

lukechilds commented Apr 30, 2019

Needed to do a stricter Promise check cos it was causing downstream tests to fail in assert.

@goto-bus-stop
Copy link
Member

lets goooo! thanks for all the work! i'll do some final touchups for Node 0.10 and then release

@goto-bus-stop goto-bus-stop merged commit 5abeabe into browserify:master May 1, 2019
@lukechilds
Copy link
Member Author

Awesome!

@goto-bus-stop
Copy link
Member

📦 0.12.0

lukechilds added a commit to browserify/commonjs-assert that referenced this pull request May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants