More robust logic for various edge cases

[lionel-rowe]

Fixes #26
Fixes #15

Started out with some tweaks but turned into a rewrite.

• is-callable dependency is no longer needed, as "callable" strictly means functions plus the single special case of document.all. typeof document.all isn't function so it returns false anyway.
• Checking is now significantly more robust, e.g. lots of cases with misleading => or function.
• All the old tests are retained, but rewritten somewhat.
• I inlined the make-arrow-function, make-async-function, and make-generator-function test deps.
• Added new test groups, e.g. one each for the 2 GitHub issues, plus async generator functions, object properties, object methods, classes, and various combinations.

[socket-security]

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: npm/[email protected], npm/[email protected], npm/[email protected]

View full report↗︎

[ljharb]

as "callable" strictly means functions plus the single special case

this is incorrect; there’s older engines in which regexes are callable.

inlined

why? That makes them not reusable. If the deps need improvement let’s improve them rather than inlining.

[ljharb]

I appreciate the effort - but i think you’re missing a lot of context. These tests, and the package, need to run on every JS engine, including ones in which new syntax would break the entire program, which is why the “make-“ deps abstract around the use of Function. We can’t directly use any syntax that’s not in ES3, which is what eslint helps enforce.

[lionel-rowe]

as "callable" strictly means functions plus the single special case

this is incorrect; there’s older engines in which regexes are callable.

Whoah, that's pretty freaky. Would an instanceof check fix it? Also, what's the purpose of supporting ES3 given that arrow functions can't syntactically exist in ES3 (or ES5 for that matter)?

inlined

why? That makes them not reusable. If the deps need improvement let’s improve them rather than inlining.

It makes the tests hard to follow if they depend on 3 separate external NPM packages that just export variables; even more so given that the exported arrays were being sliced arbitrarily within the test code without explanation.

[ljharb]

The explanation is in the git log :-) with the full list, the checks fail, and the needed fix is to have the tests pass largely unchanged, with the full list. In this case, the abstraction is worth the readability hit.

instanceof is never reliable for builtins and should be avoided. while certainly in an env that doesn’t have arrow syntax, this predicate will always return false, the predicate may need to run in an env so that code doesn’t need to check syntax support and branch.

[ljharb]

this check is handled reliably by the make- packages, and since i use them in multiple places, i still think it's better to use them than to try to inline things.

[lionel-rowe]

this check is handled reliably by the make- packages, and since i use them in multiple places, i still think it's better to use them than to try to inline things.

I'm deliberately checking for node version, as otherwise syntax errors would just get swallowed silently without testing the thing they're designed to test, even in environments that should support that syntax (it's also really tough to spot such syntax errors given the lack of syntax highlighting within the source strings). Also to be used consistently, the separate-package approach would require an NPM package for each of:

• Sync arrow functions
• Sync non-arrow functions
• Async arrow functions
• Async non-arrow functions
• Sync generators
• Async generators
• Sync methods
• Async methods
• Async generator methods
• Classes

At that point, you're reaching left-pad-levels of NPM-fueled dependency hell.

[ljharb]

I'm perfectly content with that, as the entire problem of left-pad was unpublishing, and it had literally nothing to do with lots of deps. More deps is better!

[lionel-rowe]

More deps is better!

I think that's the first time I've ever heard someone say that 😅 I'm all for farming out complex functionality to well-tested libraries, but I question farming out test fixtures to a multitude of NPM packages.

Maybe a single, external make-function package could do the trick? Something akin to this:

type FunctionSourceConfig = {
	source: string
	minNodeVersion: string
}

declare var syncArrowFunctions: FunctionSourceConfig[]
declare var syncNonArrowFunctions: FunctionSourceConfig[]
declare var asyncArrowFunctions: FunctionSourceConfig[]
declare var asyncNonArrowFunctions: FunctionSourceConfig[]
declare var syncGenerators: FunctionSourceConfig[]
declare var asyncGenerators: FunctionSourceConfig[]
declare var syncMethods: FunctionSourceConfig[]
declare var asyncMethods: FunctionSourceConfig[]
declare var asyncGeneratorMethods: FunctionSourceConfig[]
declare var classes: FunctionSourceConfig[]

var allFunctions = { syncArrowFunctions, syncNonArrowFunctions, asyncArrowFunctions, ..., classes }

declare function hydrate(fnSource: FunctionSourceConfig): Function

export function list(key) {
	return hydrate(allFunctions[key])
}
export function makeFunction (key) {
	return list(key)[0]
}

minNodeVersion may be handy as I'm noticing some failing tests that seem to be due to old node versions stringifying differently in certain edge cases (e.g. ({ "x =>"() {} })["x =>"] stringifies as "x =>"() {} and is correctly detectable as non-arrow in newer Node versions, but may be nigh-impossible to handle in Node < ~10 as it stringifies to x =>() {} there)

[ljharb]

that's a really good suggestion for all of those make- packages to use a make-function package as a core, but the value I find here includes pre-populating a list of fixtures.

I also use them in https://npmjs.com/es-value-fixtures, which i use in a lot of my projects' tests.

[lionel-rowe]

I also use them in https://npmjs.com/es-value-fixtures, which i use in a lot of my projects' tests.

I wasn't aware of that package, and I can certainly see the value of having a comprehensive, centralized set of fixtures (with multiple entrypoints if needed, which would negate the need for multiple packages). For the purposes of this PR, I've split fixtures.js into its own file. Feel free to propagate those changes to the upstream dependencies as you see fit, though if it was me I'd probably put everything in es-value-fixtures and remove anything upstream of it.

It's worth mentioning that the existing test dependencies aren't a good abstraction for the purposes of this package anyway, as this existing test shows:

is-arrow-function/test/index.js

Lines 68 to 73 in 264503a

	forEach(asyncFuncs.slice(0, 2), function (asyncFunc) {
	t.ok(isArrowFunction(asyncFunc), 'async arrow function ' + asyncFunc + ' is arrow function');
	});
	forEach(asyncFuncs.slice(2), function (asyncFunc) {
	t.notOk(isArrowFunction(asyncFunc), 'async non-arrow function ' + asyncFunc + ' is not an arrow function');
	});

The first two of make-async-function are supposed to return true and any others to return false, a correspondence that would be liable to break as soon as you added more functions to make-async-function.