Skip to content

Conversation

koddsson
Copy link
Contributor

@koddsson koddsson commented Jul 29, 2019

@koddsson koddsson requested a review from a team July 29, 2019 10:37
index.d.ts Outdated
@@ -0,0 +1,3 @@
export default class MarkdownToolbarElement extends HTMLElement {
readonly field?: HTMLTextAreaElement;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why readonly: field?: TextAreaElement here instead of readyonly field: HTMLTextAreaElement | undefined like https://github.com/github/remote-input-element/pull/6/files#diff-b52768974e6bc0faccb7d4b75b162c99R2?

I also noticed that

import MarkdownToolbarElement from '.'
const element = document.querySelector('*')

if (element instanceof MarkdownToolbarElement) {
  element.field.value = element.field.value.trim()
}

does not fail TS check but it does fail in Flow. Is this expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I thought initially that the only way to do nullable types was by using the union | undefined but I've since found out that appending ? to the argument is a shorthand of | undefined:

Ref: https://www.typescriptlang.org/docs/handbook/advanced-types.html#optional-parameters-and-properties

You do need to set strictNullChecks though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does not fail TS check but it does fail in Flow. Is this expected?

How are you running the checks here? I don't get a error in either flow or typescript :/

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested TS by adding a test.ts in the same directory. I tested Flow in github/github responsive-markdown-toolbar.js since it's a module import definition.

So I thought initially that the only way to do nullable types was by using the union | undefined but I've since found out that appending ? to the argument is a shorthand of | undefined:

I think we should be consistent across modules or we should have specific reasons to choose one over the other.

get field(): ?HTMLTextAreaElement {
const id = this.getAttribute('for')
if (!id) return
const field = document.getElementById(id)
return field instanceof HTMLTextAreaElement ? field : null
}
}

In this case we might actually return HTMLTextAreaElement | undefined | null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should be consistent across modules or we should have specific reasons to choose one over the other.

Sounds good. I think we should use the more descriptibe | undefined | null everywhere since we don't know what strictNullChecks will be set to in the application that consumes the element and so we can return null like you mentioned.

@koddsson koddsson merged commit 33c0857 into master Jul 30, 2019
@koddsson koddsson deleted the add-typescript-definition-file branch July 30, 2019 15:42
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