Uh oh!
There was an error while loading. Please reload this page.
fix(dts): add missing types to support resizable arraybuffer - #54637
fix(dts): add missing types to support resizable arraybuffer#54637Indrajeet Nikam (indrajitbnikam) wants to merge 3 commits into
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Indrajeet Nikam (indrajitbnikam)
commented
Jun 13, 2023
@microsoft-github-policy-service agree |
Indrajeet Nikam (indrajitbnikam)
commented
Jun 13, 2023
Also, Can you help me understand how to fix this test case? https://github.com/microsoft/TypeScript/actions/runs/5258441307/jobs/9502673967?pr=54637#step:5:137 |
Joshua Chen (Josh-Cena)
left a comment
There was a problem hiding this comment.
- You should be adding this to esnext.d.ts, not es5.
- You need to do the same for
SharedArrayBuffer.
Bruce Pascoe (fatcerberus)
commented
Jun 14, 2023
Now that you mention it, why is |
Because... ArrayBuffer is es5? For example it was first in Chrome v7, far before es6.
If you use |
It’s not. It’s not mentioned anywhere in the ES5.1 spec AFAICT and I specifically remember it not being supported in many ES5 engines such as Duktape until it got standardized with ES6, which was a pain point for me at the time. |
Joshua Chen (Josh-Cena)
commented
Jun 14, 2023
Mmm, you are right—it's implemented around 2010, which is after ES5 already. Still it does seem older than most ES6 APIs (Promise, Map, Symbol, etc.) |
Kenta Moriuchi (petamoriken)
commented
Jun 14, 2023
Indrajeet Nikam (@indrajitbnikam) TypeScript's How about the following:
|
Kenta Moriuchi (petamoriken)
commented
Jun 14, 2023
Historically |
Jake Bailey (jakebailey)
commented
Jun 14, 2023
Adding an entirely new |
Indrajeet Nikam (indrajitbnikam)
commented
Jun 14, 2023
Hi Jake Bailey (@jakebailey) I've changed these files as you can see in my comment above. |
Jake Bailey (jakebailey)
commented
Jun 14, 2023
Ah, sorry, missed the second message. The errors in the editor are necessarily provided by the version of TypeScript provided by VS Code or in As for the git problem, it's likely that I broke something in |
Jake Bailey (jakebailey)
commented
Jun 14, 2023
Yep, my bad. Sent #54644. |
Jake Bailey (jakebailey)
commented
Jun 14, 2023
For now, you can manually |
Indrajeet Nikam (indrajitbnikam)
commented
Jun 14, 2023
Thanks for clarifying so quickly 😊 |
Uh oh!
There was an error while loading. Please reload this page.
b0f736a to
5925906Compare5925906 to
e675236CompareAlso, I have another question about these already existing types, Have a look at this Existing code MDN Doc (here) How to cover this scenario? |
Should I do it the way I've done it over here for all missing constructor signatures for Frankly speaking, I think this can be fixed with a single type as follows. interfaceSharedArrayBufferConstructor{new(byteLength?: number,options?: SharedArrayBufferOptions): SharedArrayBuffer}What do you think? |
Since ES2017, not only So to modify |
Hi Kenta Moriuchi (@petamoriken) , Do you want me to do that change or we are just discussing it? Also, I'm not sure how to fix the failing tests. Can you help me understand it? |
Indrajeet Nikam (@indrajitbnikam)
This is just my opinion, I think there needs to be a decision by TypeScript members.
The baseline test seems to be failed. Baselines are used to manage if there are any changes in the expected output of the TypeScript compiler. Baselines are located in
Please see https://github.com/microsoft/TypeScript/blob/main/CONTRIBUTING.md#managing-the-baselines |
Indrajeet Nikam (indrajitbnikam)
commented
Jun 17, 2023
Hi Kenta Moriuchi (@petamoriken),
Ohh, I get it. By the way, Thanks a lot for helping me understand why tests were failing. I had missed reading that section of |
Indrajeet Nikam (indrajitbnikam)
commented
Jun 17, 2023
Hi Kenta Moriuchi (@petamoriken) Can you help me understand what should be described here in this related issue for this PR? Should it be |
Kenta Moriuchi (petamoriken)
commented
Jun 18, 2023
Yes, that's right. |
Indrajeet Nikam (indrajitbnikam)
commented
Jun 19, 2023
Hi Bruce Pascoe (@fatcerberus)Joshua Chen (@Josh-Cena)Jake Bailey (@jakebailey) , Can someone check the PR and let me know if I need to change something? |
| /** | ||
| * Read-only. The maximum length that this ArrayBuffer can be resized to (in bytes). | ||
| */ | ||
| readonly maxByteLength: number; |
There was a problem hiding this comment.
I suggest using get maxByteLength(): number unless the team thinks otherwise; ditto for the other occurences.
There was a problem hiding this comment.
I've seen the readonly as well in multiple places. But yeah, let's wait for the team to comment on it.
There was a problem hiding this comment.
I agree with Joshua Chen (@Josh-Cena), the spec example uses accessors, so it makes sense to do so here.
(It's quite likely that old readonlys should be changed to accessors, but that might break backward compatibility, eg classes extend lib interfaces and override properties.)
There was a problem hiding this comment.
I agree with Joshua Chen (@Josh-Cena), the spec example uses accessors, so it makes sense to do so here.
(It's quite likely that old
readonlys should be changed to accessors, but that might break backward compatibility, eg classes extend lib interfaces and override properties.)
That seems like a breaking change we might want to take, though perhaps only if we can find a way to only enable it when --useDefineForClassProperties is true or the target it sufficiently high enough to warrant it.
Indrajeet Nikam (indrajitbnikam)
commented
Jun 20, 2023
Oh Sorry, did not realise that. Thanks for reviewing though 😊. |
Nathan Shively-Sanders (sandersn)
left a comment
There was a problem hiding this comment.
This needs a test in tests/cases/conformance/esnext that shows off example uses of the new entries. The file should be named esnextArrayBuffer.ts or similar, and the first line should be
// @lib: esnext.arraybufferso that the test harness will use the right lib setting
| ["esnext.array", "lib.es2023.array.d.ts"], | ||
| ["esnext.collection", "lib.es2023.collection.d.ts"], | ||
| ["esnext.symbol", "lib.es2019.symbol.d.ts"], | ||
| ["esnext.arraybuffer", "lib.esnext.arraybuffer.d.ts"], |
There was a problem hiding this comment.
Ron Buckton (@rbuckton) Do you have an opinion abut we should add a separate lib entry for arraybuffer? This feels like the kind of thing that went in to earlier es20xx.collection entries.
There was a problem hiding this comment.
Resizable ArrayBuffers and growable SharedArrayBuffers are Stage 4 and will be in ES2024 when it is standardized, so a lib.es2024.arraybuffer.d.ts might make sense at this point, though it would still only be referenced from esnext since we don't have a --target es2024 yet..
There was a problem hiding this comment.
Or a lib.es2024.collections.d.ts, rather. I agree it makes more sense to try to keep the names somewhat consistent.
| /** | ||
| * Read-only. The maximum length that this ArrayBuffer can be resized to (in bytes). | ||
| */ | ||
| readonly maxByteLength: number; |
There was a problem hiding this comment.
I agree with Joshua Chen (@Josh-Cena), the spec example uses accessors, so it makes sense to do so here.
(It's quite likely that old readonlys should be changed to accessors, but that might break backward compatibility, eg classes extend lib interfaces and override properties.)
| interface SharedArrayBuffer { | ||
| /** | ||
| * Read-only. Whether this SharedArrayBuffer can be grow or not. |
There was a problem hiding this comment.
| *Read-only.WhetherthisSharedArrayBuffercanbegrowornot. | |
| *ReturnstrueifthisSharedArrayBuffercangrow. |
Although, growable is self-explanatory, and the current explanation is redundant with it. If we must have a jsdoc here, I'd prefer to see an explanation of context or what different properties a growable SharedArrayBuffer has, or a link to an explanation.
| } | ||
| /** | ||
| * ArrayBuffer constructor options |
There was a problem hiding this comment.
I would delete this. It doesn't add anything to the name of the type.
| readonly maxByteLength: number; | ||
| /** | ||
| * Read-only. Whether this ArrayBuffer can be resized or not. |
There was a problem hiding this comment.
| *Read-only.WhetherthisArrayBuffercanberesizedornot. | |
| *ReturnstrueifthisArrayBuffercanberesized. |
Actually, my comments about growable apply; I'd prefer to delete this comment.
| * Resizes the ArrayBuffer to the specified size (in bytes). | ||
| * @param newLength The new length, in bytes, to resize the ArrayBuffer to. | ||
| */ | ||
| resize(newLength: number): undefined; |
There was a problem hiding this comment.
I would call this newByteLength like the proposal does. Then consider deleting the jsdoc.
| @@ -0,0 +1,58 @@ | |||
| interface ArrayBuffer { | |||
| /** | |||
| * Read-only. The maximum length that this ArrayBuffer can be resized to (in bytes). | |||
There was a problem hiding this comment.
The spec proposal points out that this is equal to byteLength if options.maxByteLength is not provided. I'd stay closer to the example's explanation:
| *Read-only.ThemaximumlengththatthisArrayBuffercanberesizedto(inbytes). | |
| *IfthisArrayBufferisresizable,returnsthemaximumbytelengthgivenduringconstruction;returnsthebytelengthifnot. |
Lino Le Van (lino-levan)
commented
Aug 9, 2023
Indrajeet Nikam (@indrajitbnikam) are you willing to push this forward? If not, I'd be willing to take this on. |
Seir (seirdotexe)
commented
Oct 17, 2023
Any updates? Node 20 LTS is coming soon, so would be nice if the types are there, too 😊 |
Dominik Moritz (domoritz)
commented
Dec 16, 2023
Indrajeet Nikam (@indrajitbnikam) are you still working on this? This is currently blocking me from cleanly adopting reusable buffers. |
Indrajeet Nikam (indrajitbnikam)
commented
Dec 17, 2023
via email
Hi Dominik,
Im not working on this at the moment. You can take it forward from here.
Thanks & Regards,
Indrajeet Nikam …On Sat, 16 Dec 2023 at 11:08 PM, Dominik Moritz ***@***.***> wrote:
@indrajitbnikam <https://github.com/indrajitbnikam> are you still working
on this? This is currently blocking me from cleanly adopting reusable
buffers.
—
Reply to this email directly, view it on GitHub
<#54637 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AF6UTX2SINLOEYPTUCA43WTYJXMCDAVCNFSM6AAAAAAZFGFJVWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNJYHA3TSMRSGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Dominik Moritz (domoritz)
commented
Dec 17, 2023
Okay. Unfortunately, I don't have cycles to fix this right now. I hacked around the types in apache/arrow#39252 for now. |
To help with PR housekeeping, I'm going to close this PR since it is stale and there's an updated version at #57858. |








Fixes#54636
I am opening this PR to fix the missing(not updated) type for ArrayBuffer. This PR is being raised due to this discussion in denoland/deno issue here.