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

report(tsc): infer createElement type from tag name #6637

Merged
merged 2 commits into from
Nov 27, 2018
Merged

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented Nov 23, 2018

This is a little silly, but there's a small quality of life improvement, it closes an old TODO, and it eliminates a decent number of type assertions.

Bases the created element type on the tag name passed in, so e.g. DOM.createElement('a') returns an HTMLAnchorElement, not just a generic Element. If the tag is unknown, falls back to just HTMLElement, which is close enough to correct for us today.

Uses the same tag-name-to-element-type map that tsc uses for document.createElement, just with a union for the HTMLElement fallback instead of the function overload they use (and since DOM.createElement calls document.createElement internally, this type is checked by their type).

@brendankenny brendankenny changed the title report(tsc): infer createElement type from tag name report(tsc): infer createElement type from tag name Nov 23, 2018
@@ -364,8 +364,7 @@ class ReportUIFeatures {
// load event, however it is cross-domain and won't fire. Instead, listen
// for a message from the target app saying "I'm open".
const json = reportJson;
window.addEventListener('message', function msgHandler(/** @type {Event} */ e) {
const messageEvent = /** @type {MessageEvent} */ (e);
Copy link
Member Author

@brendankenny brendankenny Nov 23, 2018

Choose a reason for hiding this comment

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

no idea what was going on here. Maybe fixed on the compiler/built-in defs side so it was not only inferred, but inferred as a MessageEvent.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

I'm always down for fewer type assertions!

@@ -18,6 +18,8 @@

/* globals URL self */

/** @typedef {HTMLElementTagNameMap & {[id: string]: HTMLElement}} HTMLElmentByTagName */
Copy link
Collaborator

Choose a reason for hiding this comment

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

HTMLElementTagNameMap is the built-in you're talking about?

Copy link
Member Author

Choose a reason for hiding this comment

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

HTMLElementTagNameMap is the built-in you're talking about?

yes

/**
* @param {string} name
* @template {string} T
* @param {T} name
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we type this as the keys of the HTMLElementTagNameMap?

Copy link
Member Author

Choose a reason for hiding this comment

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

can we type this as the keys of the HTMLElementTagNameMap?

We could, though it would eliminate the fallback to the generic HTMLElement when a tag name isn't recognized. Maybe we're ok with that in exchange for better completion/stricter checking?

Copy link
Collaborator

Choose a reason for hiding this comment

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

would we still get helpful completion if it were strict options | string? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

strict options | string

sadly no, it's smart about it and coalesces to just string.

Copy link
Member Author

@brendankenny brendankenny Nov 27, 2018

Choose a reason for hiding this comment

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

ok, I tried to use just HTMLElementTagNameMap, and we have one place that's unrecognized: 'summary' in

const summaryEl = this.dom.createChildOf(groupEl, expandable ? 'summary' : 'div');

When Edge gets <details> support I imagine that they'll also support <summary> and their WebIDL will get updated and so the tsc types will get updated (I'm surprised it already has 'details' support, honestly), but until then we probably just want to go with how this PR is now.

@brendankenny brendankenny merged commit 5f36915 into master Nov 27, 2018
@brendankenny brendankenny deleted the tsctag branch November 27, 2018 20:59
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.

2 participants