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

Polyfilled test selector optimizations #949

Merged
merged 3 commits into from
Jan 4, 2021

Conversation

izelnakri
Copy link
Contributor

  • In the first commit we introduce a polyfill for ie11 to support Array.from and Array.prototype.find.
  • In the second commit we optimize findAll() and waitFor to use Array.from
  • In the third commit we optimize triggerKeyEvent to use array.find() instead of array.filter(predicate)[0];

@scalvert
Copy link
Contributor

scalvert commented Dec 6, 2020

Thanks, @izelnakri. Would you mind rebasing/resolving conflicts?

@izelnakri
Copy link
Contributor Author

@scalvert rebased 👍

@scalvert
Copy link
Contributor

scalvert commented Dec 6, 2020

Thanks! One thing I like better about this approach than the first PRs is that once we eventually drop support for IE11, we can just delete the polyfills.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

I worry a little bit about automatically polyfilling these array methods. If a consuming app that intends to support IE11 adds some usage in their app of [].find (for example) the tests will pass (even if they are testing against IE11) but when deployed the app will throw/fail.

🤔

@izelnakri
Copy link
Contributor Author

izelnakri commented Dec 9, 2020

This is a good point i think but bit rare case. We could make them exported find() and/or toArray() from the same module name, thats an option. Solves both issues, what do you think @rwjblue?

export function find(array, func) {
  return array?.find?.(func) || findFunction(array, func);
  // or:
  return Array.prototype.find ? array.find(func) : findFunction(array, func); // this is probably more performant / i like this more
}

export function toArray(array, func) {

}

@rwjblue
Copy link
Member

rwjblue commented Dec 11, 2020

Ya, that seems good to me @izelnakri. Thank you!

@izelnakri izelnakri force-pushed the ie-11-polyfills branch 2 times, most recently from e124cb2 to 94c6873 Compare December 12, 2020 16:46
@izelnakri
Copy link
Contributor Author

@rwjblue updated the changes.

In the CI tests are passing, however for other CI actions there is a weird broccoli specific error that I couldn't understand. It doesn't seem to be related to the changes, would appreciate if you take a look: https://github.com/emberjs/ember-test-helpers/pull/949/checks?check_run_id=1542886060#step:5:121

@scalvert
Copy link
Contributor

@izelnakri this was a bug in ember-auto-import if I'm not mistaken. I think there's been a fix that's shipped already. It looks like it was released in 1.9.0 of that package.

@scalvert
Copy link
Contributor

@izelnakri if you rebase this branch onto master I think you should be g2g.

@izelnakri
Copy link
Contributor Author

@scalvert done, thanks!

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for working through this @izelnakri!

@rwjblue rwjblue merged commit aa44cbc into emberjs:master Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants