Skip to content

Return types of Object.assign #45034

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

Closed
lionel-rowe opened this issue Jul 15, 2021 · 6 comments
Closed

Return types of Object.assign #45034

lionel-rowe opened this issue Jul 15, 2021 · 6 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@lionel-rowe
Copy link
Contributor

lionel-rowe commented Jul 15, 2021

Bug Report

πŸ”Ž Search Terms

Object.assign

πŸ•— Version & Regression Information

4.3.2

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about Object.assign

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

type Foo = { bar: number }

// expected return type: Foo
// actual return type:   Foo & { bar: number }
const createFoo = (obj: Foo) =>
    Object.assign(obj, {
        bar: 1,
    })

// expected return type: String & { __html: string }
// actual return type:   string & { __html: string }
const markSafe = (html: string) =>
    Object.assign(html, {
        __html: html,
    })

πŸ™ Actual behavior

This may be 2 separate bugs, but I suspect they may be related.

  1. For createFoo above, the returned type specifies & { bar: number }, even though { bar: number } is already part of the Foo type.
  2. For markSafe above, the returned type specifies string, even though what is actually returned is a String object with the additional property added, not a primitive string (so, for example, markSafe('') !== markSafe('')).

πŸ™‚ Expected behavior

  1. createFoo return type is Foo.
  2. markSafe return type is String & { __html: string }.
@fatcerberus
Copy link

In the first case the intersection could be collapsed but I don’t think it’s possible to represent the second case in the type system. Object.assign() has to be generic and I don’t think there’s a type operator to represent boxing a primitive type.

@RyanCavanaugh
Copy link
Member

Type display is allowed to choose among equivalent representations so this isn't a bug. Not eagerly collapsing unions/intersections is generally the behavior preferred by users and is computationally cheaper, so this is expected or at least justifiable.

I agree with @fatcerberus there isn't a better representation of what to do with primitive passed into Object.assign. This is rarely done intentionally and it doesn't seem like a problem that needs solving.

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Jul 15, 2021
@lionel-rowe
Copy link
Contributor Author

lionel-rowe commented Jul 15, 2021

Type display is allowed to choose among equivalent representations so this isn't a bug. Not eagerly collapsing unions/intersections is generally the behavior preferred by users and is computationally cheaper, so this is expected or at least justifiable.

Might help if I show my use case for this rather than an abstract example:

export const createCanvas = (width: number, height: number) =>
	globalThis.OffscreenCanvas
		? new OffscreenCanvas(width, height)
		: Object.assign(document.createElement('canvas'), { width, height })

Creates an OffscreenCanvas if available in the environment, otherwise an HTMLCanvasElement. The OffscreenCanvas constructor takes width and height params, whereas an HTMLCanvasElement can only be constructed with document.createElement, which doesn't take those params (the attributes are usually assigned imperatively), hence the use of Object.assign.

To be honest it's a minor irritation and can be fixed by adding a type annotation, but definitely seems like the naively expected behavior in this case would be that the function's return type would simply be OffscreenCanvas | HTMLCanvasElement.

Also, you get no indication that there's a bug in this modified version:

export const createCanvas = (widht: number, heigth: number) =>
	globalThis.OffscreenCanvas
		? new OffscreenCanvas(widht, heigth)
		: Object.assign(document.createElement('canvas'), { widht, heigth })

Not saying that modified version should be a type error (it shouldn't), but it'd be helpful if there was some easily-visible way in which its results differed from the correct version.

@fatcerberus
Copy link

HTMLCanvasElement & { width: number, height: number } should be functionally equivalent to HTMLCanvasElement in every way - if it's not, then that sounds like a bug.

@lionel-rowe
Copy link
Contributor Author

Yes, it's functionally equivalent. The only difference is that intellisense displays the type as HTMLCanvasElement & { width: number, height: number }.

@sirian
Copy link
Contributor

sirian commented Aug 13, 2021

I think assign<T, U>(target: T, source: U): T & U; signature is wrong.
Probably it should be assign<T, U>(a: T, b: U): Omit<T, keyof U> & U;

image
https://www.typescriptlang.org/play?#code/MYewdgzgLgBAhjAvDA3gDwFwwIwF8DcAUKJLAEZKqYwBEAZiCDboceNDMJQPJkBWAU2BQAdHAgQAlgHMwACjgAaGGQCU+GAHpNnGJIhUsYAQDcBAJxaE4ItCKggAylHOSw0uesJlb9py7cPL2BfB2dXd08iQgATIQAbOHMBGDoAVzBhSXB4CRkwAB4AFWUAVQA+BSwSlSxS1SxuAFtJKGLlAGsBAE8QOhgKmAAyAeiSDhjKcSlZBWU1DW0YSf1DGGgI6RYY0P9NqNjfeIF3KAALfCA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

4 participants