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

[FEATURE] Stable types for @ember/polyfill #20325

Conversation

theroncross
Copy link

toward #19946

Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

Excellent! This also needs the corresponding entries (and files!) removed from the preview types. Once you do that, ping me and I'll re-review and land it. Thank you!

@chriskrycho chriskrycho added enhancement TypeScript Work on Ember’s types labels Dec 19, 2022
@theroncross
Copy link
Author

theroncross commented Dec 20, 2022

Ah, that step is missing from #19946 (comment)

In this case, the preview types are an improvement over the source types more precise than the types for Object.assign. However, they have a dependency on the Mix types from /types/preview/@ember/polyfills/types.d.ts, which don't seem to be used anywhere else. Do you have a preference for where one-off utility types live?

OR

Should I update the types (and tests) to match those in the lib.es2015.core.d.ts for Object.assign and call it good?

return Object.assign(target, ...rest);

@chriskrycho
Copy link
Contributor

I think updating to match the types for Object.assign() is probably good. It's annoying, but also we won't carry it around for very long—it’s gone in 5.x. I'll update the instructions; thanks for nothing that!

@chriskrycho
Copy link
Contributor

@theroncross are you still up to finish updating this? Would love to land it!

The updated types and tests now match those of `Object.assign`

toward emberjs#19946
@theroncross theroncross force-pushed the types-migration-ember-polyfills branch from b82b4c4 to dd50deb Compare January 19, 2023 03:11
@theroncross
Copy link
Author

Sorry about that. Got sidetracked by life a bit.

The tests needed some changes to match the Object.assign type behavior, but it should be close now.

@chriskrycho
Copy link
Contributor

Sweet, will take a look tomorrow, and just re-approved CI! Thank you!

@@ -7,7 +7,7 @@ import { deprecate } from '@ember/debug';
export function assign<T, U>(target: T, source: U): T & U;
export function assign<T, U, V>(target: T, source1: U, source2: V): T & U & V;
export function assign<T, U, V, W>(target: T, source1: U, source2: V, source3: W): T & U & V & W;
export function assign(target: object, ...sources: object[]): object;
export function assign(target: object, ...sources: any[]): any;
Copy link
Contributor

Choose a reason for hiding this comment

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

:sigh: This is probably the “right” thing to do here, since it’s what TS itself has, but I hate it.

@chriskrycho chriskrycho changed the title Move @ember/polyfill types to stable [FEATURE] Stable types for @ember/polyfill Jan 21, 2023
@chriskrycho
Copy link
Contributor

@theroncross looks good overall but I think some of the type tests will need updating!

@chriskrycho
Copy link
Contributor

@theroncross I think we're going to end up closing this as "not needed" since it's all removed as of Ember v5. Thank you again, though!

@chriskrycho
Copy link
Contributor

Superseded here specifically by #20380.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement TypeScript Work on Ember’s types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants