saucelabs / node-saucelabs

A wrapper around Sauce Labs API
Apache License 2.0
93 stars 44 forks source link

suggested various refactors #136

Open boneskull opened 2 years ago

boneskull commented 2 years ago

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).
christian-bromann commented 2 years ago
  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 😊

boneskull commented 2 years ago

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

enriquegh commented 2 years ago

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.

boneskull commented 2 years ago

(notes to self about TS refactoring)