Skip to content

Updates Uint8ArrayConstructor to match MDN documentation. #38449

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

Merged
merged 3 commits into from
Jul 30, 2020

Conversation

MicahZoltu
Copy link
Contributor

@MicahZoltu MicahZoltu commented May 10, 2020

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array/Uint8Array

new Uint8Array(); // new in ES2017
new Uint8Array(length);
new Uint8Array(typedArray);
new Uint8Array(object);
new Uint8Array(buffer [, byteOffset [, length]]);

While the previous constructors aren't significantly different from the new, they would prevent someone from doing new Uint8Array(myBuffer, undefined, undefined) since there was no overload that accepted undefined as a second parameter and buffer as first.
Fixes #38446

Fixes #

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array/Uint8Array
```
new Uint8Array(); // new in ES2017
new Uint8Array(length);
new Uint8Array(typedArray);
new Uint8Array(object);
new Uint8Array(buffer [, byteOffset [, length]]);
```
While the previous constructors aren't significantly different from the new, it would prevent someone from doing `new Uint8Array(myBuffer, undefined, undefined)` since there was no overload that accepted undefined as a second parameter and buffer as first.
Fixes microsoft#38446
@seishun
Copy link

seishun commented May 10, 2020

Perhaps the same change should be applied to all other TypedArrays.

@MicahZoltu
Copy link
Contributor Author

MicahZoltu commented May 10, 2020

Perhaps the same change should be applied to all other TypedArrays.

Done.

@sandersn sandersn added the For Backlog Bug PRs that fix a backlog bug label May 20, 2020
@orta
Copy link
Contributor

orta commented Jul 30, 2020

OK, this looks good to me 👍

@orta orta merged commit b601487 into microsoft:master Jul 30, 2020
@ExE-Boss
Copy link
Contributor

ExE-Boss commented Jul 30, 2020

This also fixes this case:

export class DataBlock extends Uint8Array {
	constructor(sizeOrBuffer, ...restArgs) {
		if (sizeOrBuffer instanceof ArrayBuffer) {
			// fine.
			super(sizeOrBuffer, ...restArgs);
		} else {
			Assert(typeof sizeOrBuffer === 'number');
			super(sizeOrBuffer);
		}
	}
}

Which previously failed for reasons similar to #14107.

@sandersn
Copy link
Member

sandersn commented Aug 3, 2020

Because this splits up ArrayLike<number> | ArrayBufferLike, this is a breaking change: It prevents passing exactly Array<number> | ArrayBuffer, for example.

I think this is fairly rare, but it happens once in a large internal Microsoft codebase.

sandersn added a commit that referenced this pull request Aug 3, 2020
PR #38449 changed the second overload of the constructor for Int8,
Uint8, Int32, Uint32, Int16, Uint16, Float, Float64 -Array so that it no
longer includes `ArrayBufferLike`. It's just `ArrayLike<number>`.
This is fine except in the case that
the caller provides exactly `ArrayLike<number> | ArrayBufferLike`. This
PR adds ArrayBufferLike back so that the union is once again accepted.

This avoids a breaking change, in particular in one Microsoft-internal
codebase.
sandersn added a commit that referenced this pull request Aug 4, 2020
PR #38449 changed the second overload of the constructor for Int8,
Uint8, Int32, Uint32, Int16, Uint16, Float, Float64 -Array so that it no
longer includes `ArrayBufferLike`. It's just `ArrayLike<number>`.
This is fine except in the case that
the caller provides exactly `ArrayLike<number> | ArrayBufferLike`. This
PR adds ArrayBufferLike back so that the union is once again accepted.

This avoids a breaking change, in particular in one Microsoft-internal
codebase.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Uint8Array constructor doesn't accept possibly undefined argument
5 participants