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

type of Document.body getter should be nullable #44772

Closed
trusktr opened this issue Jun 27, 2021 · 7 comments
Closed

type of Document.body getter should be nullable #44772

trusktr opened this issue Jun 27, 2021 · 7 comments
Labels
Needs More Info The issue still hasn't been fully clarified

Comments

@trusktr
Copy link

trusktr commented Jun 27, 2021

body: HTMLElement;

Before the body is ready, the value is null.

PR: #44773

Also, although TypeScript now has ability for a getter to have a different type than a setter, it is still not possible to make the type properly. Based on Chromes behavior this is what describes it:

class Document {
  get body(): HTMLBodyElement | HTMLFramesetElement | null { ... }
  set body(value: HTMLBodyElement | HTMLFramesetElement) { ... }
}

however such a type is not allowed in TS 4.3.

If we follow the deprecation of HTMLFramesetElement, which I think is a good idea, then the type would be this:

class Document {
  get body(): HTMLBodyElement | null { ... }
  set body(value: HTMLBodyElement) { ... }
}

but that still doesn't work.

Also, we could ignore the frameset deprecation and just write

class Document {
  get body(): HTMLElement | null { ... }
  set body(value: HTMLElement) { ... }
}

which also doesn't work.

This leads to runtime errors if someone is type-checking a <script> tag before body is ready.

@trusktr trusktr changed the title type of Document.body should be nullable type of Document.body getter be nullable Jun 27, 2021
@trusktr
Copy link
Author

trusktr commented Jun 27, 2021

Here's a trick to make it work:

class Document2 extends Node {
	// @ts-expect-error
	get body(): HTMLBodyElement | null {
		return {} as any
	}
	set body(el: HTMLBodyElement) {
	}
}

const b: HTMLBodyElement = new Document2().body // error (good), getter can return null

new Document2().body = null // error (good), setter accepts only non-null

playground

@andrewbranch
Copy link
Member

Are you asking for #43662 in general, or are you actually asking for the type of body to be changed in the DOM lib typings, which incidentally may require something like #43662? The former would be a duplicate, and the latter would be a discussion for https://github.com/microsoft/TypeScript-DOM-lib-generator (but would likely be resolved as “technically correct but the ratio of pain to value is way, way beyond what we would ever consider shipping”).

@andrewbranch andrewbranch added the Needs More Info The issue still hasn't been fully clarified label Jun 28, 2021
@fatcerberus
Copy link

I don't understand why a getter that returns T | null is considered incompatible with a setter that takes T? That setup makes perfect sense to me--the thing starts out uninitialized, but you have to provide a valid value the first time you invoke the setter. I get that the idea is for foo.x = foo.x to always work, but that already fails in other cases, e.g. for optional properties under strictOptionalProperties (or whatever it's since been renamed to).

@andrewbranch
Copy link
Member

I’m totally sympathetic to that reasoning and we’ve seen a number of examples from the DOM that can’t be described by today’s variant accessor implementation. I’d encourage both of you to post examples like these in #43662.

@trusktr trusktr changed the title type of Document.body getter be nullable type of Document.body getter should be nullable Jul 28, 2021
@trusktr
Copy link
Author

trusktr commented Jul 28, 2021

@andrewbranch Hello, thanks for the reply. Indeed, I am not asking about #43662, but just noting that the actual type of document.body is HTMLElement | null when getting, and HTMLElement when setting (otherwise runtime error if null is passed in).

But related to #43662, as @fatcerberus mentioned, it is not currently possible to denote such a type in TS.

I think I would just make it | null (for both set and get, as a single field) and just let the runtime error teach people not to pass null in.

@trusktr
Copy link
Author

trusktr commented Jul 28, 2021

Or, if @ts-expect-error is an ok thing to put in the lib definition, that previous workaround I gave would be better. :)

Based on the fact that the @ts-expect-error seems to allow the typing to work, it seems that enabling broader types for getters is merely the removal of a conditional check somewhere.

@andrewbranch
Copy link
Member

Feel free to start a discussion at https://github.com/microsoft/TypeScript-DOM-lib-generator, but I am sticking with my analysis that this would be

technically correct but the ratio of pain to value is way, way beyond what we would ever consider shipping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs More Info The issue still hasn't been fully clarified
Projects
None yet
Development

No branches or pull requests

3 participants