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

regression: array.includes no longer working. #263

Closed
jamestalmage opened this issue Nov 23, 2015 · 16 comments
Closed

regression: array.includes no longer working. #263

jamestalmage opened this issue Nov 23, 2015 · 16 comments
Labels
bug current functionality does not work as desired

Comments

@jamestalmage
Copy link
Contributor

Reference: sindresorhus/serialize-error#3 (comment)

The following no longer works:

test(t => {
  var keys = Object.keys(obj);
  var hasName = keys.includes('name');
  t.true(hasName);
  t.end();
});

I get the following error:

TypeError: keys.includes is not a function

We are using the runtime plugin, to avoid any polyfills, so it makes sense that Array.prototype.includes does not actually exist. I would have thought the runtime plugin would have picked this up and provided it via some helper function.

We are still on Babel 5, so I am punting until we land Babel 6 support before digging in.

// @sebmck @thejameskyle

@jamestalmage
Copy link
Contributor Author

So it looks like the last time this passed, was on ava@0.3.0.
At that point we were using babel-core/register, which loads polyfills.

So maybe the runtime plugin has never handled this.

@vadimdemedes vadimdemedes added the bug current functionality does not work as desired label Nov 23, 2015
@jamiebuilds
Copy link
Contributor

I would have thought the runtime plugin would have picked this up and provided it via some helper function.

Babel can't resolve instance methods on references to polyfills.

Think about it this way, how can a compiler know what keys is in this case? Until it knows that it can't possibly know what includes is referring to.

keys.includes()

There is the potential in this particular case to do something about it because we can infer that the result of Object.keys is an array. However, if you had a case like the following we couldn't possibly get it working.

function includesName(keys) {
  return keys.includes('name');
}

This inability to do things consistently is why Babel will never auto-polyfill like this.

@jamestalmage
Copy link
Contributor Author

Couldn't you transform it to something like this:

var x = arr.includes('foo');

// =>

var x = includesHelper(arr, 'name');

function includesHelper(maybeArray, ...args) {
  if (Array.isArray(maybeArray) && !maybeArray.includes) {
    // use polyfill
  } else {
   return maybeArray.includes.apply(maybeArray, args);
  }
}

Obviously there is a performance hit for this way (especially when expanded to accommodate String.includes) . Is that the only reason it is considered unacceptable, or are there other reasons?

@jamiebuilds
Copy link
Contributor

If you're going to include the code to polyfill it, why not just polyfill it?

Also, I don't think we'd ever be interested in going down that route in core, you're welcome to try your hand at in in your own plugin.

@jamestalmage
Copy link
Contributor Author

If you're going to include the code to polyfill it, why not just polyfill it?

AVA is trying to provide as much ES2015+ support inside tests, while not polluting the production environment at all. ES2015 convenience in the tests, while still allowing you to write really tiny, dependency-less modules that do not require a build step. I thought the goals of the optional runtime plugin were similar.

@sindresorhus
Copy link
Member

Wonder if the vm module could help in this situation.

@jamestalmage
Copy link
Contributor Author

Interesting. Looking back at the 0.10 docs, it seems there is some historical flakiness. It would probably be pretty painful.

The list of prototype polyfills is not that long. A plugin might be easier.

@thejameskyle - Does the runtime plugin provide non polluting polyfills for static methods? Array.of, String.fromCodePoint, etc?

@jamiebuilds
Copy link
Contributor

Babel can actually do that through babel-plugin-transform-runtime

https://github.com/babel/babel/blob/master/packages/babel-plugin-transform-runtime/src/definitions.js#L25

These are aliases to core-js functions:

Array.includes;
import _Array$includes from "babel-runtime/core-js/array/includes";
_Array$includes;

@jamestalmage
Copy link
Contributor Author

That's what I thought. So, if I were to write a plugin to fill in the gaps for AVA, I would only need to tackle instance methods. transform-runtime handles globals (Promise, Map, etc) and static instance methods (Array.of, etc).

@ariporad
Copy link
Contributor

@jamestalmage: I really don't think it would be that big of a deal to have the polyfill... I can't think of any possible reason (if I knew that the tests ran in ES2015), that the polyfill would confuse me.

@sindresorhus
Copy link
Member

@ariporad The problem with such polyfill is that it mutates the global Array prototype, so it would influence not just your test code, but the code being tested.

@novemberborn
Copy link
Member

Is this really a bug? I agree it's a gotcha but even if we did an optimistic transform like @jamestalmage suggested in #263 (comment) that wouldn't work for test helpers.

@novemberborn
Copy link
Member

Closing, this is not something we can fix when transpiling test files. Users could add a --require for babel-polyfill but AVA shouldn't do that by default.

@sindresorhus
Copy link
Member

@novemberborn Maybe we should document it somewhere? It can be surprising behavior.

@novemberborn
Copy link
Member

Maybe we should document it somewhere? It can be surprising behavior.

Yes. Opened #727.

@danny-andrews
Copy link
Contributor

Late to this party but I just wanted to affirm the decision to have AVA not add polyfills by default (global polluting or no). Jest took a different route (and includes babel-polyfill by default) which led to tests passing code which broke in production. BAD BAD BAD!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug current functionality does not work as desired
Projects
None yet
Development

No branches or pull requests

7 participants