Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

suggested various refactors #136

Open
2 of 5 tasks
boneskull opened this issue Jan 7, 2022 · 4 comments
Open
2 of 5 tasks

suggested various refactors #136

boneskull opened this issue Jan 7, 2022 · 4 comments

Comments

@boneskull
Copy link
Contributor

boneskull commented Jan 7, 2022

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. 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. 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
Copy link
Contributor

  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
Copy link
Contributor Author

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

@enriquegh
Copy link
Contributor

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
Copy link
Contributor Author

(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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants