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

AsyncFunction and GeneratorFunction issues #103

Open
doasync opened this issue Aug 11, 2017 · 5 comments
Open

AsyncFunction and GeneratorFunction issues #103

doasync opened this issue Aug 11, 2017 · 5 comments

Comments

@doasync
Copy link

doasync commented Aug 11, 2017

I think, tests should pass:

const type = require('type-detect');
const assert = require('assert');

const asyncFn = async () => {};
const generatorFn = function* generator () {};

function getTag(obj) {
  return Object.prototype.toString.call(obj).slice(8, -1);
}

assert(type(asyncFn) === getTag(asyncFn));
assert(type(generatorFn) === getTag(generatorFn));

Is it a bug? If not, you should add it to README.

@doasync doasync changed the title AsyncFunction, GeneratorFunction AsyncFunction and GeneratorFunction bugs Aug 11, 2017
@doasync doasync changed the title AsyncFunction and GeneratorFunction bugs AsyncFunction and GeneratorFunction issues Aug 11, 2017
@keithamus
Copy link
Member

Hey @doasync thanks for the issue

This, again, is because of the early typeof optimisation; async () = >{} and function *() {} are all typeof x === "function", and so we return that early:

type-detect/index.js

Lines 54 to 57 in c7895e4

var typeofObj = typeof obj;
if (typeofObj !== 'object') {
return typeofObj;
}

We could probably change that to:

  var typeofObj = typeof obj;
  if (typeofObj !== 'object' && typeofObj !== 'function') {
    return typeofObj;
  }

We'd welcome a PR, which includes that change plus you'll need to change the tests for generatorfunction, which are here:

itIf(supportGenerators)('generator function', function () {
assert(type(eval('function * foo () {}; foo')) === 'function'); // eslint-disable-line no-eval
});
itIf(supportGenerators)('generator', function () {
assert(type(eval('(function * foo () {}())')) === 'Generator'); // eslint-disable-line no-eval
});

Also, on top of that, this lib does not have tests for async functions as they weren't around when the lib was written - so you'll need to add tests for that.

@shvaikalesh
Copy link
Contributor

shvaikalesh commented Aug 13, 2017

Related issues: chaijs/chai#958 and chaijs/chai#949.

I don't think there are enough good use cases for differentiating async/generator functions from regular ones: any function can return promise or iterator. If we change this, we will break pretty common x.should.be.a("function") and y.should.respondTo("foo") usages.

If you really wish to detect async/generator functions, you can use @@toStringTag:

;(function* () {})[Symbol.toStringTag].should.equal("GeneratorFunction")
;(async function() {})[Symbol.toStringTag].should.equal("AsyncFunction")

@keithamus
Copy link
Member

If we change this, we will break pretty common x.should.be.a("function") and y.should.respondTo("foo") usages.

We could easily add a bit of extra code to chai to prevent breaking changes for the .a assertion; along the lines of:

isFunction = expectedType === 'function' && typeof thing === 'function'
if (isFunction) {
  assert(true) 
} else {
  assert(type(thing) === expected)
}

respondTo shouldn't be called with async/generator functions anyway; it is meant for classes and their constructors. Although we'd probably still want to ensure this was a non-breaking change.

Granted, this adds more complexity in chai; but I am somewhat compelled by the following points:

  • @doasync is right, functions aren't primitives
  • We're mostly using typeof as a performance optimisation, and leaning much more heavily on toStringTag semantics. This would follow suit
  • Differentiation of these types could be helpful in testing
  • The point of this lib is to avoid code like ;(function* () {})[Symbol.toStringTag]; (perhaps a controversial statement:) developers should not need to know the intimate mechanics of JS to test their code

At any rate, there's more discussion needed here. I'll remove the pr wanted tag and let's get other @chaijs/type-detect members to weigh in.

@meeber
Copy link
Contributor

meeber commented Aug 18, 2017

I don't think there's a clear right or wrong answer here. I lean slightly toward the idea that it's reasonable for type-detect to distinguish between different types of functions.

@keithamus
Copy link
Member

I would argue that type-detects role is really a fancy polyfill for @@toStringTag. Almost all of the code that isn't returning just [Symbol.toStringTag] is for performance or for conformance - arguably that includes the typeof check at the beginning. Framed this way, it seems to make a lot more sense for type-detect to distinguish between different types of functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants