Rewrite to Typescript + fully Webpack build #111
Conversation
sergeyshmakov
commented
Mar 22, 2019
sergeyshmakov
commented
- 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.
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.
Nit: JSON shouldn't have trailing commas
There was a problem hiding this comment.
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.
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 |
