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

Adds change-array-by-copy polyfills #1285

Merged
merged 14 commits into from
May 3, 2023
Merged

Adds change-array-by-copy polyfills #1285

merged 14 commits into from
May 3, 2023

Conversation

mhassan1
Copy link
Collaborator

This PR adds the following ES2023 polyfills:

  • Array.prototype.toReversed
  • Array.prototype.toSorted
  • Array.prototype.toSpliced
  • Array.prototype.with
  • %TypedArray%.prototype.toReversed
  • %TypedArray%.prototype.toSorted
  • %TypedArray%.prototype.with

Array.prototype.toReversed
Array.prototype.toSorted
Array.prototype.toSpliced
Array.prototype.with
%TypedArray%.prototype.toReversed
%TypedArray%.prototype.toSorted
%TypedArray%.prototype.with
@mhassan1 mhassan1 requested a review from a team as a code owner April 17, 2023 17:52
@github-actions github-actions bot added the library Relates to an Origami library label Apr 17, 2023
@origamiserviceuser origamiserviceuser added this to Backlog in Origami ✨ Apr 17, 2023
@romainmenke
Copy link
Collaborator

/ok-to-test sha=cf9230b

@romainmenke
Copy link
Collaborator

CI running here: https://github.com/Financial-Times/polyfill-library/actions/runs/4724256232

CI is likely still broken, but maybe it also has useful feedback :)

@romainmenke
Copy link
Collaborator

romainmenke commented Apr 21, 2023

/ok-to-test sha=cab69e6

https://github.com/Financial-Times/polyfill-library/actions/runs/4762911502

@mhassan1
Copy link
Collaborator Author

https://github.com/Financial-Times/polyfill-library/actions/runs/4762911502

There are a bunch of failures:

  • F must be a constructor: The ArrayBuffer polyfill does not expose int8Array.constructor.name
  • With == with: Missing dependency on Function.prototype.name
  • A.sort is not a function: The ArrayBuffer polyfill includes TypedArray.prototype.sort, but we don't deliver ArrayBuffer to browsers that have TypedArray but don't have TypedArray.prototype.sort (e.g. Chrome 44)
  • [TypedArray].prototype.sort: argument is not a Function object: We should not pass undefined to sort

All of these have been fixed except A.sort is not a function, which may require some discussion. I think we have a few options:

  1. Deliver the ArrayBuffer polyfill to versions that do not have TypedArray.prototype.sort
  2. Write a separate TypedArray.prototype.sort polyfill
  3. Implement TypedArray.prototype.sort inside TypedArray.prototype.toSorted

@JakeChampion
Copy link
Owner

Financial-Times/polyfill-library/actions/runs/4762911502

There are a bunch of failures:

* `F must be a constructor`: The `ArrayBuffer` polyfill does not expose `int8Array.constructor.name`

* `With == with`: Missing dependency on `Function.prototype.name`

* `A.sort is not a function`: The `ArrayBuffer` polyfill includes `TypedArray.prototype.sort`, but we don't deliver `ArrayBuffer` to browsers that have `TypedArray` but don't have `TypedArray.prototype.sort` (e.g. [Chrome 44](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/sort#browser_compatibility))

* `[TypedArray].prototype.sort: argument is not a Function object`: We should not pass `undefined` to `sort`

All of these have been fixed except A.sort is not a function, which may require some discussion. I think we have a few options:

1. Deliver the `ArrayBuffer` polyfill to versions that do not have `TypedArray.prototype.sort`

2. Write a separate `TypedArray.prototype.sort` polyfill

3. Implement `TypedArray.prototype.sort` inside `TypedArray.prototype.toSorted`

Thanks for the detailed write-up! I'm in favour of option 2. It has precedent as we took that option for Promise.prototype.finally as well -- https://github.com/Financial-Times/polyfill-library/blob/master/polyfills/Promise/prototype/finally/config.toml#L9

@mhassan1
Copy link
Collaborator Author

I've added TypedArray.prototype.sort. This is ready for re-test.

@romainmenke
Copy link
Collaborator

/ok-to-test sha=bde70e8

@mhassan1
Copy link
Collaborator Author

I've addressed a couple more failures:

  • Use Object.getPrototypeOf instead of __proto__ for IE10 and below
  • Use new instead of Construct in Safari 10 and below, which has typeof Int8Array === 'object'

This is ready for re-test.

@romainmenke
Copy link
Collaborator

/ok-to-test sha=22b3791

@JakeChampion
Copy link
Owner

@romainmenke
Copy link
Collaborator

CI all green 🎉

@JakeChampion JakeChampion merged commit 908553b into JakeChampion:master May 3, 2023
9 checks passed
Origami ✨ automation moved this from Backlog to Done May 3, 2023
@mhassan1 mhassan1 deleted the change-array-by-copy branch June 30, 2023 12:35
@robertboulton robertboulton removed this from Done in Origami ✨ Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
library Relates to an Origami library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants