-
-
Notifications
You must be signed in to change notification settings - Fork 222
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
Rewrite to Typescript + fully Webpack build #111
Conversation
sergeyshmakov
commented
Mar 22, 2019
- Rewriten on Typescript to get more strict typings
- Fixed IE11 iterate of NodeList's objects (crashes because NodeList not iterable and not have Array methods like forEach)
- Rewriten build fully with Webpack to get more simple workflow
- Tested in Chrome, IE 11, Edge, Firefox
@gregnb thoughts? |
@MatthewHerbst @gregnb Hey guys! How are you? =) |
I'm good thanks, hope you are well too :) It's @gregnb's package so I'll let him have the first say on this. |
Sorry guys, been really busy. This is pretty awesome. @MatthewHerbst what do you think? I'll leave the approval up to you |
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.
Looks good to me. I'm not too familiar with TS but I don't see anything that concerns me. Wouldn't mind one more pair of eyes on it.
@cmckni3 you started the initial TS work. Would you mind giving this a brief look over please?
}, | ||
"include": [ | ||
"./src" | ||
"./src", |
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.
Nit: JSON shouldn't have trailing commas
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 agree with this suggestion. This could be problematic. iirc npm
cli used to bomb out on these.
EDIT Noticed it's in tsconfig.json
. TypeScript compiler is forgiving but I would change just in case.
"dom.iterable" | ||
], | ||
"importHelpers": true, | ||
"jsx": "react", | ||
"declaration": true, | ||
"removeComments": true, | ||
"downlevelIteration": true, | ||
"allowSyntheticDefaultImports": true, |
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.
Nit: JSON shouldn't have trailing commas
Sweet, looks like TypeScript is a go. This is what I envisioned the PR to look like. 👍 To separating the webpack configs for dev/prod as well. Good catch with the iterable bugs @sergeyshmakov. That's why TypeScript was complaining and I had forced it |
Thanks for the super quick response @cmckni3! |
@gregnb once you're ready to release a new version (I think this is only a minor version chance since the Public API is unchanged) feel free to merge. Thanks for the work on this @sergeyshmakov! |
No problem! Thanks for the mention. I would close #105 in favor of this. |
One thing to note: |
All set published. Thanks everyone |