Skip to content

fix: remove "apply" from enablements whitelist#475

Merged
erights merged 1 commit into
masterfrom
unlist-apply
Sep 24, 2020
Merged

fix: remove "apply" from enablements whitelist#475
erights merged 1 commit into
masterfrom
unlist-apply

Conversation

@erights

@erights erights commented Sep 23, 2020

Copy link
Copy Markdown
Contributor

Fixes #474

Removes Function.prototype.apply from the enablements whitelist, of properties that need the override mistake fixed. This particular entry said it was because tape broke. But @ljharb , the maintainer of tape, said that it should not have a problem with apply. After removing it, our existing tests work just fine. But (question for reviewers) do any of them use tape in a way that would detect a remaining problem if there is one?

@ljharb ljharb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The only thing I can think of is that a prior stack trace incorrectly pointed to tape (the user's presumed test runner) as the cause, which means that this PR would again break them, at which point we could triage the stack trace to find the proper cause of the error.

@warner warner left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can't remember how tape interacted with this. I do vaguely remember differences between tape version 4 and version 5, and maybe we were unable to use version 5 because it had problems that 4 did not.

We're using AVA on agoric-sdk now, not tape, so that won't be a source of compatibility data anymore.

I'd say remove it and then wait for something else to indicate a compatibility problem.

@erights erights merged commit b52f8d2 into master Sep 24, 2020
@erights erights deleted the unlist-apply branch September 24, 2020 08:16
@ljharb

ljharb commented Sep 26, 2020

Copy link
Copy Markdown

tape 5's breaking change was respecting a promise returned from the test callback; it shouldn't be possible for v4 vs v5 to have affected this.

(sorry to hear you're no longer using tape :-( )

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.

Should we stop suppressing the override mistake for Function.prototype.apply?

3 participants