node-saucelabs icon indicating copy to clipboard operation
node-saucelabs copied to clipboard

suggested various refactors

Open boneskull opened this issue 3 years ago • 4 comments

Taking a look at the state of the codebase, I'd like to recommend the following changes:

  1. [ ] 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.
  2. [ ] Configure babel-env to use shippedProposals: true, which should replace most of the one-off babel plugins.
  3. [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.
  4. [x] Adopt prettier, eslint-plugin-prettier, husky and lint-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.
  5. [ ] (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 than this, so we'll need to find a different way to accomplish the same thing).

boneskull avatar Jan 07 '22 21:01 boneskull

  1. 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 😊

christian-bromann avatar Jan 10 '22 11:01 christian-bromann

yeah, somebody should work on that bind operator. would be useful. alas...

boneskull avatar Jan 10 '22 20:01 boneskull

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.

enriquegh avatar Apr 22 '22 18:04 enriquegh

(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 the SauceLabs constructor unless the target satisfies the type of SauceLabs. Currently, the target is a new object with some props in it; instead, we'll want the target to be this. 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.

boneskull avatar May 10 '22 00:05 boneskull