style: first start to move this package to TypeScript #479
Conversation
| @@ -0,0 +1,11 @@ | |||
| { | |||
There was a problem hiding this comment.
This is the smallest tsconfig possible for now (from https://www.typescriptlang.org/docs/handbook/migrating-from-javascript.html). Only addition is strict.
| @@ -3015,7 +3015,7 @@ function isJsonBuffer(obj) { | |||
| * | |||
| * @param path {Array} Passed from hook, but unused (because empty where this | |||
| * function is used, since we aren't keeping track of it for effiency). | |||
| * @param val {...} The object to reject. | |||
| * @param val {any} The object to reject. | |||
There was a problem hiding this comment.
This fails otherwise with:
src/types.js:3018:16 - error TS1014: A rest parameter must be last in a parameter list.
3018 * @param val {...} The object to reject.
~~~
Found 1 error in src/types.js:3018
| @@ -43,18 +43,22 @@ | |||
| "node": ">=6.0.0" | |||
| }, | |||
| "scripts": { | |||
| "prepublish": "rimraf lib && tsc", | |||
There was a problem hiding this comment.
this generates lib on local install & before publish for now. Only difference is that lib is git ignored.
| @@ -1,21 +0,0 @@ | |||
| let crypto = require('crypto'); | |||
There was a problem hiding this comment.
This one was first moved in 8483c05 and then changed, however git doesn't show it as moved anymore due to the extension + content change.
| @@ -0,0 +1,20 @@ | |||
| import { createHash } from 'crypto'; | |||
There was a problem hiding this comment.
This generates to https://gist.github.com/joscha/b2f96866e4fb29c9dfadec7472ef0eb6 - you can see how close it is to the original lib/platform.js
joscha
changed the title
| "compilerOptions": { | ||
| "outDir": "./lib", | ||
| "allowJs": true, | ||
| "target": "es5", |
There was a problem hiding this comment.
Do we need to downlevel to ES5?
There was a problem hiding this comment.
My intent with this pull request is to keep everything stable. The current JS code is ES5, so we should stay there.
Eventually we can adjust this.
There was a problem hiding this comment.
Don't get me wrong, I think we should change this, but the current minimum node version defined for this package is 6.x, so there are some discussions to be had before we can make this a es2030 ;)
There was a problem hiding this comment.
The code, at least in the master branch, uses ES6 features since #422 (classes, let/const, probably more).
There was a problem hiding this comment.
You're right. I also just checked https://node.green/ - even on 6.x (I doubt anyone would still be using it and I don't think we should support anything smaller than 18.x) we can use most features.
@mtth - are you opposed to making this es6 - it would mean some exports, etc. would change and JS code generated from the typescript would be slightly different.
|
@joscha - I wish you gave me a heads up before putting effort in. After thinking about it, I think this migration is probably best done by me: to streamline maintenance, I would want to use the same TypeScript standard setup as my other projects. |
I did in to which I got and then |
|
Yep, sorry - I didn't mean to imply that creating a PR would be a good next step here. |
Well you know, open source would do that. I also think transforming it step by step with TS and JS side by side would make this much easier and allow other people to contribute. |
This adds typescript and transforms the smallest part (
platform.js) to Typescript.It keeps everything else the same, including locations and ES level. This allows TS and JS to co-exist for the time being.
via #461 (comment)
relates to: #423, #128, #219