remove gulp, fix ENOENT bug, typecheck stuff #84
Conversation
boneskull
force-pushed
the
boneskull/no-gulp
branch
from
e1bb17e to
3c02988
Compare
|
I can also just extract the |
boneskull
force-pushed
the
boneskull/no-gulp
branch
from
3c02988 to
009211f
Compare
| "@babel/preset-env", | ||
| { | ||
| "targets": { | ||
| "node": "14" |
There was a problem hiding this comment.
is it ok to have min node version set separately for each module?
There was a problem hiding this comment.
I'm not sure what you're asking in this context, but generally speaking our minimum version is 14. in this case, it's @appium/support's minimum version, and that's a dependency of teen_process, so it can't be any older. Each project (not package) will necessarily have its own Babel config.
| @@ -29,6 +30,31 @@ | |||
| "lib", | |||
| "build/lib" | |||
| ], | |||
| "scripts": { | |||
There was a problem hiding this comment.
linking @KazuCocoa : this should probably be done for all other dependencies, so we could eventually completely get rid of gulp usage
boneskull
force-pushed
the
boneskull/no-gulp
branch
from
009211f to
11dd582
Compare
| @@ -120,7 +175,7 @@ class SubProcess extends EventEmitter { | |||
| // comes in chunks and a line could come in two different chunks, so | |||
| // we have logic to handle that case (using this.lastLinePortion to | |||
| // remember a line that started but did not finish in the last chunk) | |||
| for (const [streamName, streamData] of _.toPairs(streams)) { | |||
| for (const [streamName, streamData] of /** @type {[['stdout', string], ['stderr', string]]} */(_.toPairs(streams))) { | |||
There was a problem hiding this comment.
fwiw, this cast is here because _.toPairs() is not strictly typed. the keys stdout and stderr in streams are string literals, meaning they cannot be anything else--however, _.toPairs() obliterates this and instead what we get back is of type [[string, string], [string, string]] which of course loses detail
There was a problem hiding this comment.
I think Object.entries() (which would be comparable) also suffers from this. We can use Object.entries if we want, since we're targeting node.js v14
| @@ -19,7 +17,7 @@ describe('exec', function () { | |||
| let {stdout, stderr, code} = await exec(cmd, args); | |||
| stdout.should.contain('exec-specs.js'); | |||
| stderr.should.equal(''); | |||
| code.should.equal(0); | |||
| should.equal(code, 0); | |||
There was a problem hiding this comment.
this needs to change because code is number|null, and if it is the latter, code.should.equal(0) would throw an exception, since null is not an object and certainly not one having a should property.
This PR removes gulp and adds much of the same tooling and strategies used by `appium`. "New" stuff includes: - Added `prettier` (but did not enable nor run it) - Added type checks (but did not make part of build step) - `lint-staged` for automatic `eslint --fix` as a pre-commit hook Also sorted `package.json`
The `errno` property of a `NodeJS.ErrnoException` is always a `number`, and `code` is what we're looking for. At some point the behavior changed, and this looks to have been a holdover from old Node.js versions. The relevant test made too general of an assertion, which is partly why it was not noticed.
- Ensure various instance properties are guaranteed to be defined - Ensure boolean-ness of various options - Avoid deprecated `fs.R_OK` When appium/appium#17345 is published and we upgrade `@appium/support`, `tsc` will pass and we can add it to the build.
boneskull
force-pushed
the
boneskull/no-gulp
branch
from
11dd582 to
4e48706
Compare
See the commit messages for more detail