Add exports.types

[lukechilds]

This is required for https://github.com/browserify/commonjs-assert/pull/44

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

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

// CC @goto-bus-stop @BridgeAR

[lukechilds]

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]

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: https://github.com/defunctzombie/node-util/pull/33

[lukechilds]

@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]

@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]

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

[BridgeAR]

@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]

@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]

@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]

OK, thanks a lot for your help @BridgeAR.

I've implemented all type checks apart from:

• isProxy
• isExternal
• isModuleNamespaceObject
• isNativeError

I'm struggling with these, any help would be appreciated.

isProxy

I'm not sure this is even possible from userland, I think we need access to V8 to know if an object is a proxy.

It is impossible to determine whether an object is a proxy or not

• http://2ality.com/2014/12/es6-proxies.html

Is it possible or should we not implement this?

isExternal

Again, I'm not sure but I don't think this is possible to implement from userland.

isModuleNamespaceObject

This seems to be testing if a value is an ES6 module namespace:

import * as ns from './a.js';

util.types.isModuleNamespaceObject(ns);  // Returns true

I don't think this will work in Node.js without a flag and I don't think browser support is very good. Does it make sense to implement this?

isNativeError

I'm not sure of a reliable way to implement this. This is required by assert though. Any ideas on how this can be implemented?

[lukechilds]

@goto-bus-stop any idea why the Travis builds aren't failing?

I added a test file which is expectedly failing for some Node.js versions: https://travis-ci.org/defunctzombie/node-util/jobs/523848722

However the Travis build still succeeds when the tests fail because "npm test" exits with 0.

I haven't changed the tests, just added a new file. Shouldn't this be catching my failing test file and throwing a non zero exit code: https://github.com/defunctzombie/node-util/blob/6e048d9eca8e06931c203aebcb9ddd9213a64b96/test/node/index.js#L9

Or do I need to add something else to me test file?

[BridgeAR]

@lukechilds I would implement isNativeError as isNativeError = isError for now. The other types can not be matched against and I would implement them as non-enumerable properties that throw an error in case they are used (Something like new Error('Proxies can not be detected in userland')).

[vweevers]

Shouldn't this be catching my failing test file and throwing a non zero exit code

@lukechilds I think the issue is there's no callback here, meaning any error is swallowed:

https://github.com/defunctzombie/node-util/blob/a5a2f846817e34d2f1bee8a02034ee23954918b3/test/node/index.js#L14-L21

Should be:

series([..], function (err) {
  if (err) throw err
})

[lukechilds]

That did it, thanks @vweevers!

[lukechilds]

I'm now doing feature detection before setting the properties so we can give errors when features aren't supported so all exports are set programatically:

https://github.com/defunctzombie/node-util/blob/93be5d72c66a0dbb07feb958a62ae0957fdc794e/support/types.js#L299-L307

But good point regarding function names, I will give them all names.

[lukechilds]

Just realised, we probably don't want to throw an error on all unsupported types.

e.g if BigInt is unsupported, we don't want to throw saying it's unsupported, we can just always return false ebcause we know it can never be true.

That way code will still work as normal and we know we aren't giving incorrect results.

The things we can't detect in userland should still obviously throw though.

I'll fix that.

[goto-bus-stop]

AFAIK the only "unique" thing about Module Namespaces is that their toString is [object Module]:

import * as ns from 'https://unpkg.com/dlv?module'
Object.prototype.toString.call(namespace)
// → [object Module]

[lukechilds]

But can I write browser tests for that?

[goto-bus-stop]

One idea may be to have a browser-specific test that adds a script tag… Something like:

window.onModuleNamespace = function (ns) {
  t.ok(types.isModuleNamespace(ns))
}
var script = document.createElement('script')
script.type = 'module'
script.textContent = `
  import * as x from '/test/files/x.js'
  window.onModuleNamespace(x)
`
script.onerror = t.error

With airtap, we can serve that x.js file to the browser tests: https://github.com/airtap/airtap/blob/master/doc/airtap.yml.md

e; actually easier would be to use dynamic import instead of a script tag

// try/catch/eval for if the syntax is not supported
try {
  eval('import("/test/files/x.js")').then((ns) => {
    types.isModuleNamespace(ns)
    // etc
  })
} catch (err) {
  t.comment('import is not supported in this environment')
  t.pass()
}

[lukechilds]

@goto-bus-stop ok cool, I'll give that a go.

[lukechilds]

Tests are failing in Node.js v4 due to:

  var TypedArrayProto_toStringTag =
      uncurryThis(
        Object.getOwnPropertyDescriptor(TypedArrayPrototype,
                                        Symbol.toStringTag).get);

Because Symbol.toStringTag is undefined.

I can change the TypedArray tests to use ObjectToString = uncurryThis(Object.prototype.toString); instead which works for most tests.

However the tests case:

Object.defineProperty(new Uint8Array(),
                          Symbol.toStringTag,
                          { value: 'foo' }); }

fails because it's seen as [object foo] instead of [object Uint8Array].

Any ideas how to handle this in environments without Symbol.toStringTag?

Can we use ObjectToString and remove that test for the browserify version?

Alternatively we could use TypedArrayProto_toStringTag in environments that have Symbol.toStringTag and ObjectToString in environments that don't.

I'm not sure it's a good idea for the same package to have different behaviour in different environments though.

[BridgeAR]

@lukechilds using the fallback for environments without symbols is ideal. Before Symbol.toStringTag there was no way to manipulate that when using the original Object.prototype.toString function. Using is in combination with symbols is not failsafe though and that's why we need the different check in newer environments.

[lukechilds]

On Node.js 4 generator functions just look functions and Promises just look like objects.

> ObjectToString(function*() {})
'[object Function]'
> ObjectToString(Promise.resolve())
'[object Object]'

This looks pretty good for generator functions: https://github.com/ljharb/is-generator-function

However I haven't found anything good for promises.

Theres these:

• https://github.com/then/is-promise
• https://github.com/sindresorhus/p-is-promise

However they just check for then/catch methods, they aren't very reliable:

isPromise({then: () => {}});
// true

Is that ok to use?

[lukechilds]

Tests are all passing :tada:

Node.js 0.12 and 0.10 fail but they are already failing in master:

https://travis-ci.org/defunctzombie/node-util/jobs/473276292

It's just in master a failing test doesn't fail the Travis build, that's fixed in this PR (https://github.com/defunctzombie/node-util/pull/32/commits/fc3e1c117221c4d32cbf8c7d12b2be371ab47700)

Is it ok to remove tests for these versions?

As far as I can tell, the current published version on npm fails the test suite. This is the Node.js v0.12 build for util v0.11.1: https://travis-ci.org/defunctzombie/node-util/jobs/449318249

[lukechilds]

Hooked the Node.js tests into the browser tests:

test/browser/types.js

var test = require('tape');
var util = require('../../');

test('util.types', function (t) {
  t.doesNotThrow(() => require('../node/types'));
  t.end();
});

Hope that's ok. I think that's going to be better and more accurate than trying rewrite all the tests for the browser.

I've tested locally in latest Chrome and Firefox and all tests are passing.

As mentioned in https://github.com/defunctzombie/node-util/pull/32#issuecomment-485647175, I can't create an account on Sauce Labs.

@goto-bus-stop Are you able to create a new branch directly in this repo based of this branch so the browser tests will run in Travis? Or you can invite me to the project and I can do it.

Other than waiting to test against all browser targets and unless there's any more feedback, I think this PR is good to go.

[goto-bus-stop]

Only the owner can invite to the repository unfortunately. :( I'll create a branch for now

[goto-bus-stop]

- testing: chrome @ Mac 10.11: 27 28 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73

hmm we should probably restrict that to the latest 2 or 3 versions instead in airtap.yml :D

[lukechilds]

@goto-bus-stop Thanks for creating the branch!

Looks like the build timed out: https://travis-ci.org/defunctzombie/node-util/jobs/524909077

🤔

[goto-bus-stop]

Changing the set of chrome browsers to just latest helped, now only IE tests are timing out. most likely some unsupported JS feature is preventing the tests from running in those browsers.

Even without full sauce access the test runs can be watched here: https://app.saucelabs.com/open_sauce/user/shtylman/tests

I'll email saucelabs about a shared browserify account so we can migrate to that for assert, events, and other modules that may need it

e; Right, it's failing on the arrow function here:

[vweevers]

now only IE tests are timing out. most likely some unsupported JS feature is preventing the tests from running in those browsers.

Yeah, arrow functions:

[vweevers]

I'll email saucelabs about a shared browserify account

@goto-bus-stop You may run into concurrency issues if too many projects use the same account.

[lukechilds]

@goto-bus-stop Ok to remove Node.js v0.10 v0.12 tests as per: https://github.com/defunctzombie/node-util/pull/32#issuecomment-487006077?

[goto-bus-stop]

I don't think we get much use out of testing on 0.10 and 0.12, but if it's easy to fix them I'd prefer that. (I'll work on that on another branch, don't worry about it here)

With that latest patch the browser tests now have useful results! https://travis-ci.org/defunctzombie/node-util/jobs/525671961

[lukechilds]

Issue with IE where because we're inferring test names from the values via value.constructor.name which doesn't work in IE.

I've explicitly set all the test method names and now the tests are running in IE.

However the isMap (and probably others) test is failing due to:

ObjectToString(new Map())
//'[object Object]'

We're checking if it's '[object Map]'.

Is it ok to fall back to a value instanceof Map in this scenario?

I'm not sure how else I can test it.

We could guard the instanceof test with something like:

if (ObjectToString(new Map()) === '[object Object]')

To make sure we only fall back to instanceof when ObjectToString isn't acting as expected.

[lukechilds]

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]

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]

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

@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 (https://github.com/browserify/commonjs-assert/pull/44) finished this week and I can't start browser testing until this PR is merged and published.

[goto-bus-stop]

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]

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]

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

[lukechilds]

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]

yeah I'm fine with leaving that "broken". If a solution is found later we can do a bugfix release :woman_shrugging:

[lukechilds]

Awesome!

Ok, those Node.js issues should be fixed.

[lukechilds]

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

[lukechilds]

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

[goto-bus-stop]

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

[lukechilds]

Awesome!

[goto-bus-stop]

:package: 0.12.0