proper js API by mathieudutour · Pull Request #11 · ActivityWatch/aw-client-js

mathieudutour · 2018-07-19T02:05:15Z

I took the liberty of publishing the package on npm (I don't think you would mind since it's a different name than the one before). Happy to give you all access to it.

johan-bjareholt

Awesome! :)

This PR makes it pretty clear that it's our first time making a API library in JavaScript :p

Just a few minor comments on the change

Before merging this we need to fix compatibility with aw-webui and aw-watcher-vscode. Hopefully the API is good enough after this that we won't have to break compatibility.

johan-bjareholt · 2018-07-19T06:31:40Z

@@ -1,24 +1,25 @@
 {
-  "name": "aw-client-js",
+  "name": "activity-watch-client",


All other activitywatch libraries are named "aw-x", please don't rename it

Indeed, name should be aw-client

just to explain a bit more why I renamed it: I keep reading aw-client as aws-client which also have AWSClient class. I was tempted to rename it to just activity-watch (same reason as for dropping the js). I understand that the other libraries are aw-* but they are mostly internals. I think using a more explicit name would help a lot to improve code readability of third party code where aw-* could co-exist with aws-* (and let's be honest, aws is a giant beast and much more likely to appear ;) )

That being said, happy to change it back if the consensus is to keep aw-client

aws-client is extremely unlikely to appear in the same codebase as aw-client. aw-client is what we're going with.

johan-bjareholt · 2018-07-19T06:32:46Z

  "scripts": {
-    "compile": "tsc",
-    "test": "./test.sh"
+    "compile": "(rm -rf out || true) && tsc",


No need to rm the dir, so don't.

johan-bjareholt · 2018-07-19T06:35:36Z

    } = {};

-    constructor(clientname: string, testing: boolean, baseurl: string | undefined = undefined) {
+    constructor(clientname: string, options?: {testing?: boolean, baseURL?: string}) {


Why options object instead?

I think this is a good change, see: https://stackoverflow.com/questions/12826977/multiple-arguments-vs-options-object

Aha, so it's rather that the limitation of not being of able to use kwargs in JS can make it hard to read since you cannot annotate the args in a nice manner. In that case it seems sane.

yeah, as a rule of thumb, if you have more than 2 arguments (especially if they are optional), a options object will be more readable

johan-bjareholt · 2018-07-19T06:37:08Z


-    createBucket(bucket_id: string, type: string, hostname: string) {
-        return this.req.post('/0/buckets/' + bucket_id, {
+    ensureBucket(bucketId: string, type: string, hostname: string): Promise<{ alreadyExist: boolean }> {


johan-bjareholt · 2018-07-19T06:38:31Z

-        });
+            type,
+            hostname,
+        }).then(() => undefined);


What's the point of .then(() => undefined)?

otherwise it's returning the axios response which is an implementation detail and shouldn't be exposed

johan-bjareholt · 2018-07-19T07:23:21Z

I took the liberty of publishing the package on npm (I don't think you would mind since it's a different name than the one before). Happy to give you all access to it.

It's fine, I'd preferably want a official one named aw-client or aw-client-js later though.
Automatic CI package updates would also be cool.

ErikBjare · 2018-07-19T10:12:27Z

I took the liberty of publishing the package on npm (I don't think you would mind since it's a different name than the one before). Happy to give you all access to it.

It's fine that you did that given the namechange, but I'm not fine with you being an owner of the official package. I registered a package with the proper name (aw-client): https://www.npmjs.com/package/aw-client

ErikBjare · 2018-07-19T10:18:31Z

 ```sh
-npm install
-npm run compile
+npm install activity-watch-client


Change to npm install aw-client

ErikBjare · 2018-07-19T10:18:50Z

-
 ```javascript
-const aw_client = require('aw-client');
+const { AWClient } = require('activity-watch-client');


Again, change to require('aw-client')

ErikBjare · 2018-07-19T10:19:24Z

-const aw_client = require('aw-client');
+const { AWClient } = require('activity-watch-client');
+
+const client = new AWCLient(myClientName)


Spelling error, should be new AWClient (notice lowercase L).

ErikBjare · 2018-07-19T10:20:00Z

@@ -1,24 +1,25 @@
 {
-  "name": "aw-client-js",
+  "name": "activity-watch-client",


Indeed, name should be aw-client

ErikBjare · 2018-07-19T10:20:22Z

-    "test": "./test.sh"
+    "compile": "(rm -rf out || true) && tsc",
+    "test": "./test.sh",
+    "prepare": "npm run compile && npm run test"


ErikBjare · 2018-07-19T10:21:35Z

-    pulsetime: number;
+    onSuccess: (heartbeat: Heartbeat) => void;
+    onError: (err: AxiosError) => void;
+    pulseTime: number;


We don't camelCase pulsetime anywhere else, so probably shouldn't here either for consistency.

ErikBjare · 2018-07-19T10:31:00Z

+    constructor(clientname: string, options?: {testing?: boolean, baseURL?: string}) {
+        if (!options) {
+          options = {}
+        }


Assign a default object to the options parameter instead.

Like this:

constructor(clientname: string, options: {testing?: boolean, baseURL?: string} = {})

it's not exactly the same: if options is null, the default parameter will not be set. Only when it's undefined.

The default parameter is compiled down to if (typeof options === 'undefined') { options = {} }

Why would options ever be null? Passing null to it shouldn't be valid so not a problem.

you never know, it's going to be used by third party devs. Tbh, we should do a proper argument check: verify that the clientname is indeed a string, that the options is indeed an object, and same for all methods and provide useful error message instead of returning the error from the server.
But that's another PR ;)

If third party devs pass an invalid argument to a function that's their problem. The types are clearly specified, they should know better than to pass in null (and why would they? Makes no sense).

95% of JS devs don't use types. Believe me, JS makes you do crazy stuff, I've been bitten by it a few time already: you wrap the API with another function that returns null in some cases and boom, you end up looking for 2h why it works in some cases and no others. Not every js library is being nice about null vs undefined.
It's a simple change that doesn't impact us in any way: the code is as readable either way and it can save some headaches so let's do it?

I really think it's unnecessary. Why would you construct an options object by calling a function? Seems like overengineering around the flaws of JavaScript for an obscure case where a developer does something that would be extremely uncommon and bad practice anyway.

Just use the default parameter syntax, it's really not worth the 3 extra lines in this case (but I do appreciate the larger point you're trying to make).

Why would you construct an options object by calling a function?

Why wouldn't you? I don't think you get to decide what someone is going to do in its code haha.

bad practice anyway

Yes it's bad practice to mix null and undefined but in the past, a lot of libraries didn't really care about the difference and it's not unlikely that someone is going to use one.

it's really not worth the 3 extra lines in this case

You know that it's going to get compiled to something longer in case of the default parameter, right? haha

I really don't think it's a good idea to optimise for code size over developer experience but whatever, it's a not hill I want to die on

ErikBjare · 2018-07-19T12:24:24Z

Just wanted to say: Huge thanks for making this PR! A lot of great changes! ❤️

mathieudutour · 2018-07-19T12:28:23Z

Before merging this we need to fix compatibility with aw-webui and aw-watcher-vscode.

This should be done after merging and publishing, right? I see that now they are using it as a submodules. It would be better to use it as an npm package I believe.

ErikBjare · 2018-07-19T12:31:11Z

We shouldn't need to fix the compatibility before merging even if they they are used as submodules.

ErikBjare · 2018-07-19T13:28:55Z

Ready to merge?

more info: ActivityWatch/aw-client-js#11

proper js API

125533b

johan-bjareholt reviewed Jul 19, 2018

View reviewed changes

ErikBjare requested changes Jul 19, 2018

View reviewed changes

mathieudutour added 3 commits July 19, 2018 06:03

no need to rm if exclude the out folder

d91d0ef

don't camelCase pulsetime

1ecf389

s/activity-watch/aw

86eb360

mathieudutour added 2 commits July 19, 2018 06:50

use default parameter instead of falsy check

fb13fe2

fix typo

93e4c91

ErikBjare approved these changes Jul 19, 2018

View reviewed changes

johan-bjareholt approved these changes Jul 19, 2018

View reviewed changes

johan-bjareholt merged commit a7236f1 into ActivityWatch:master Jul 19, 2018

devzsolt added a commit to devzsolt/aw-watcher-vscode that referenced this pull request Dec 9, 2018

Update creating AWClient

b5f6ec0

more info: ActivityWatch/aw-client-js#11

johan-bjareholt pushed a commit to ActivityWatch/aw-watcher-vscode that referenced this pull request Dec 9, 2018

Update creating AWClient

f04664a

more info: ActivityWatch/aw-client-js#11

Uh oh!

Conversation

mathieudutour commented Jul 19, 2018

Uh oh!

johan-bjareholt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathieudutour Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johan-bjareholt commented Jul 19, 2018

Uh oh!

ErikBjare commented Jul 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErikBjare Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErikBjare Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErikBjare Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErikBjare commented Jul 19, 2018

Uh oh!

mathieudutour commented Jul 19, 2018

Uh oh!

ErikBjare commented Jul 19, 2018

Uh oh!

ErikBjare commented Jul 19, 2018

Uh oh!

Reviewers

mathieudutour Jul 19, 2018 •

edited

Loading

ErikBjare Jul 19, 2018 •

edited

Loading

ErikBjare Jul 19, 2018 •

edited

Loading

ErikBjare Jul 19, 2018 •

edited

Loading