-
Notifications
You must be signed in to change notification settings - Fork 40
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
Replace flow + babel with tsc #15
Conversation
Note: selectionStart/End only applies to input elements of certain types so it could be null and throws. This fallbacks to 0. https://html.spec.whatwg.org/#do-not-apply
|
||
export default function subscribe(el: Element): Subscription { | ||
export default function subscribe(el: HTMLElement): Subscription { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(1/2) This is to ensure event listeners are correctly typed.
const beginning = textarea.value.substring(0, textarea.selectionStart) | ||
const remaining = textarea.value.substring(textarea.selectionEnd) | ||
const beginning = textarea.value.substring(0, textarea.selectionStart || 0) | ||
const remaining = textarea.value.substring(textarea.selectionEnd || 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(2/2) selectionStart
and selectionEned
only applies to input elements of certain types. https://html.spec.whatwg.org/#do-not-apply
const el = document.createElement('div') | ||
el.innerHTML = html | ||
return el.querySelector('table') | ||
} | ||
|
||
function hasTable(transfer: DataTransfer): ?Element { | ||
function hasTable(transfer: DataTransfer): HTMLElement | null | void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can avoid the void
return type if you just return null
instead of the bare returns in the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is edited in the followup PR.
"dist/index.d.ts", | ||
"dist/index.esm.js", | ||
"dist/index.umd.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not all of dist/
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dist
contains *.d.ts
for all the *.ts
files even though we only outputted one.
|
||
const pkg = require('./package.json') | ||
|
||
import typescript from 'rollup-plugin-typescript2' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently typescript2
is slow but I don't mind it here. If we do mind it we could use tsc --emitDeclarationOnly
to emit a declaration file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am hoping that rollup-plugin-typescript2
will down the line be improved to only produce declaration for the output file. tsc
would not know about the final output in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM 👍
This replace Flow with TypeScript and removes babel.
Ref:
typescript2
does 🙃 , and is used by a good number of repositories and being maintained https://github.com/ezolenko/rollup-plugin-typescript2#declarations