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

fix: remove "apply" from enablements whitelist #475

Merged
merged 1 commit into from
Sep 24, 2020
Merged

Conversation

erights
Copy link
Contributor

@erights erights commented Sep 23, 2020

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?

Copy link

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

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.

Copy link
Contributor

@warner warner left a comment

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

ljharb commented Sep 26, 2020

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