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

fix(cli/dts): add typings for Change Array by copy proposal #16499

Merged
merged 7 commits into from
Nov 2, 2022

Conversation

crowlKats
Copy link
Member

Closes #16496

@crowlKats crowlKats changed the title Array by copy types feat(cli/dts): add typings for Change Array by copy proposal Oct 31, 2022
cli/dts/lib.esnext.array.d.ts Outdated Show resolved Hide resolved
cli/dts/lib.esnext.array.d.ts Outdated Show resolved Hide resolved
cli/dts/lib.esnext.array.d.ts Outdated Show resolved Hide resolved
cli/dts/lib.esnext.array.d.ts Show resolved Hide resolved
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Please update tools/update_typescript.md in point 6. about lib.esnext.array.d.ts

@sno2
Copy link
Contributor

sno2 commented Oct 31, 2022

I was also thinking, we might want to make the toSpliced follow the same suggested technique as with by allowing new types to be included in the array as the resultant array would be considered a new entity.

Edit: these changes can be upstreamed afterwards as well: microsoft/TypeScript#50333

cli/dts/lib.esnext.array.d.ts Outdated Show resolved Hide resolved
@sno2
Copy link
Contributor

sno2 commented Oct 31, 2022

(Copied from resolved comment thread)

built-in typings arent ever complex typings like that, so we will leave it to any.

I'm not sure if it's that simple why there aren't complex typings. Many of the array methods were implemented in far older type systems so they did not have the ability to use these new type features without breaking old code. I'd consider this an inflection point in improving the user-experience with types. Also, if you type this as any, then no one who cares about sufficiently safe code will want to use this new feature. Please reconsider.

Edit: There has also been a shift in the official typescript typings towards using more complex types in an effort to improve DX. Some examples:

@bartlomieju
Copy link
Member

(Copied from resolved comment thread)

built-in typings arent ever complex typings like that, so we will leave it to any.

I'm not sure if it's that simple why there aren't complex typings. Many of the array methods were implemented in far older type systems so they did not have the ability to use these new type features without breaking old code. I'd consider this an inflection point in improving the user-experience with types. Also, if you type this as any, then no one who cares about sufficiently safe code will want to use this new feature. Please reconsider.

Edit: There has also been a shift in the official typescript typings towards using more complex types in an effort to improve DX. Some examples:

@sno2 this is a stop-gap solution until TypeScript supports this feature officially. I believe it's better to use looser types now and narrow them later, than be too strict right now.

@sno2
Copy link
Contributor

sno2 commented Oct 31, 2022

I believe it's better to use looser types now and narrow them later, than be too strict right now.

Why release loose types only to break libraries and codebases a few months later? You can always add looser behavior later if the typescript team somehow chooses to use any typing instead.

@bartlomieju
Copy link
Member

I believe it's better to use looser types now and narrow them later, than be too strict right now.

Why release loose types only to break libraries and codebases a few months later?

Okay, after confering with other team members it seems it's better to go with too strict than too loose types here. Thanks for bringing this up @sno2!

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for taking care of this Leo

@bartlomieju bartlomieju changed the title feat(cli/dts): add typings for Change Array by copy proposal fix(cli/dts): add typings for Change Array by copy proposal Nov 2, 2022
@crowlKats crowlKats merged commit 32b449f into denoland:main Nov 2, 2022
@crowlKats crowlKats deleted the array_by_copy_types branch November 2, 2022 17:07
DjDeveloperr pushed a commit to DjDeveloperr/deno that referenced this pull request Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change Array by copy typings missing
4 participants