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

More robust logic for various edge cases #29

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lionel-rowe
Copy link

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.

Copy link

socket-security bot commented May 9, 2024

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

🚮 Removed packages: npm/make-arrow-function@1.2.0, npm/make-async-function@1.0.0, npm/make-generator-function@2.0.0

View full report↗︎

@ljharb
Copy link
Member

ljharb commented May 9, 2024

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.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

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.

.eslintignore Outdated Show resolved Hide resolved
.eslintrc Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
@lionel-rowe
Copy link
Author

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
Copy link
Member

ljharb commented May 9, 2024

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.

package.json Outdated Show resolved Hide resolved
test/index.js Outdated
Comment on lines 24 to 33
try {
// eslint-disable-next-line no-new-func
return new Function('return (' + objOrSource + ')')();
} catch (e) {
if (IS_MODERNISH_NODE_ENV) {
// anything we pass to this function should be valid syntax as of modern node versions
throw e;
}
// if invalid syntax in older versions, we simply ignore them
return null;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

@ljharb ljharb May 9, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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:

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.

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