Skip to content

add typescript definition file #13

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 30, 2019
Merged

add typescript definition file #13

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2438bf6
run `npm audit fix`
koddsson Jul 29, 2019
c9ec083
add typescript definition file
koddsson Jul 29, 2019
4709442
use union type to include `undefined` and `null`
koddsson Jul 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.

markdown-toolbar-element/index.js

Lines 218 to 224 in 9b21198

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.

}
49 changes: 13 additions & 36 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"repository": "github/markdown-toolbar-element",
"main": "dist/index.umd.js",
"module": "dist/index.esm.js",
"types": "index.d.ts",
"scripts": {
"clean": "rm -rf dist",
"lint": "github-lint",
Expand All @@ -22,7 +23,8 @@
],
"license": "MIT",
"files": [
"dist"
"dist",
"index.d.ts"
],
"devDependencies": {
"@babel/cli": "^7.4.4",
Expand Down