node-saucelabs
node-saucelabs copied to clipboard
suggested various refactors
Taking a look at the state of the codebase, I'd like to recommend the following changes:
- [ ] Drop any use of stage-2 or lower JS syntax (e.g., the bind operator
::
). We shouldn't use any JS that isn't guaranteed to one day actually be valid syntax. - [ ] Configure
babel-env
to useshippedProposals: true
, which should replace most of the one-off babel plugins. - [x] Change the formatting to use two (2) spaces instead of four (4), and use semicolons, which is the most typical/popular way of formatting JS.
- [x] Adopt
prettier
,eslint-plugin-prettier
,husky
andlint-staged
to perform automatic formatting and fixing as a pre-commit hook. This should eliminate any further fussing about formatting in future discussions or code reviews. - [ ] (Re-)write all docstrings as TypeScript-compatible JSDoc and use
tsc
as a type-checker. This will give us the types from TypeScript and DefinitelyTyped, but does not demand we rewrite the project in TS. We're taking this approach in Appium and I've found it works well. This shouldn't conflict with what we're doing to generate typings, as much of the API seems to be automatically generated from a swagger doc. However, this will likely require a few refactors to keep TS happy (e.g., you cannot return anything from a constructor other thanthis
, so we'll need to find a different way to accomplish the same thing).
- Drop any use of stage-2 or lower JS syntax (e.g., the bind operator
::
). We shouldn't use any JS that isn't guaranteed to one day actually be valid syntax.
💯 I was a fan of this bind operator but don't use it these days either due to what you described 😊
yeah, somebody should work on that bind operator. would be useful. alas...
My two cents on the bind operator:
As someone with Node experience but in no way an Node expert the barrier of entry for me to contribute is way higher. I've done it but not without me bonking my head multiple times.
I also may be doing something wrong but debugging issues with node-brk is harder as well.
(notes to self about TS refactoring)
- Most of the types (see
build/index.d.ts
) are generated from the OpenAPI docs, but they don't seem to be quite right (I'm seeing a lot of object-type function parameters which are not well-defined). - It may be possible to replace our hand-rolled script with a package which does the conversion (e.g., openapi-typescript). This will almost certainly be a breaking change, even if it does fix the bugs in the current generation script.
- As mentioned, we cannot return a
Proxy
object from theSauceLabs
constructor unless the target satisfies the type ofSauceLabs
. Currently, the target is a new object with some props in it; instead, we'll want the target to bethis
. To avoid breaking things, we can leverage private fields. The compiled code will be somewhat different, but I don't think this will cause any breakage in practice.