Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

refactor: align with eslint rules #2 #520

Merged
merged 5 commits into from
Sep 28, 2021
Merged

refactor: align with eslint rules #2 #520

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
96c4ac0
refactor: align with eslint rules
fatadel Sep 20, 2021
f2be1ca
refactor: align with PR review
fatadel Sep 21, 2021
efab043
refactor: align with PR review
fatadel Sep 22, 2021
ca1e2a1
refactor: align with PR review
fatadel Sep 22, 2021
16c9ca8
refactor: align with PR review
fatadel Sep 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/binding-coap/src/coap-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ export default class CoapClient implements ProtocolClient {
return true;
}

public setSecurity = (metadata: Array<TD.SecurityScheme>) => true;
public setSecurity = (metadata: Array<TD.SecurityScheme>): boolean => true;

private uriToOptions(uri: string): CoapRequestConfig {
const requestUri = url.parse(uri);
Expand Down
14 changes: 9 additions & 5 deletions packages/binding-coap/src/coap-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,11 @@ export default class CoapServer implements ProtocolServer {

private readonly port: number = 5683;
private readonly address: string = undefined;

private readonly server: any = coap.createServer((req: any, res: any) => {
this.handleRequest(req, res);
});

private readonly things: Map<string, ExposedThing> = new Map<string, ExposedThing>();
private servient: Servient = null;

Expand Down Expand Up @@ -200,7 +202,7 @@ export default class CoapServer implements ProtocolServer {
} else {
console.info("[binding-coap]", `CoapServer failed to destroy thing with thingId '${thingId}'`);
}
resolve(removedThing != undefined);
resolve(removedThing !== undefined);
});
}

Expand Down Expand Up @@ -324,7 +326,7 @@ export default class CoapServer implements ProtocolServer {
});
// observeproperty
} else {
var oInterval = setInterval(function () {
const oInterval = setInterval(function () {
thing
.readProperty(segments[3])
// property.read() periodically
Expand All @@ -344,6 +346,9 @@ export default class CoapServer implements ProtocolServer {
res.write(content.body);

res.on("finish", function (err: Error) {
if (err) {
console.error(err);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For logging we need to use our convention, I am wondering if we can enforce it somehow with eslint 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "our convention"?
I hope you've understood that I've added that code because of this rule - https://eslint.org/docs/rules/handle-callback-err.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you take a look at our packages and the console logs you will notice that we add a prefix to the error so that we can easily detect from which binding the error came from...

e.g.,

https://github.com/danielpeintner/thingweb.node-wot/blob/59fe3264bb4cfa19a3c8cddb0f1c68b4685a4a47/packages/binding-coap/src/coap-server.ts#L72-L74

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes exactly, can you add that prefix, please? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aah okay, sure! I thought you mean there is some kind of convention for the way we handle callback errors.

Will be fixed now 😊

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. It is used elsewhere in the file also. Or do I miss anything?

Copy link
Member

@relu91 relu91 Sep 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed it. But a short question - won't there be a problem with referencing this?

Actually, he might be right, cause the whole code is wrapped inside function in this case if I recall correctly this points to the function object. @fatadel can you do a couple of tests with a small script reproducing the situation? Then we can merge this! 😃

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while I was here I did the test myself 😄 :

class Test {
    constructor() {
        this.title = "Test";
    }
    testMeFunction(){
        setInterval( function () {
            console.log("function", this.title);
        }, 1000);
    }

    testMeArrow() {
        setInterval( () => {
            console.log("arrow",this.title);
        }, 1000);
    }
}

new Test().testMeFunction();
new Test().testMeArrow();

output:

function undefined
arrow Test
function undefined
arrow Test
function undefined

arrow Test

Consequently, I'd ask @fatadel to change function into an arrow function :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I guess we need to check other places than also...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@relu91, done!
Should I also check for this inside functions in other places or let's leave it for a separate PR?

}
clearInterval(oInterval);
res.end();
});
Expand Down Expand Up @@ -485,16 +490,15 @@ export default class CoapServer implements ProtocolServer {
packet.payload = "";
packet.reset = false;
packet.ack = true;
packet.token = new Buffer(0);
packet.token = Buffer.alloc(0);

res._send(res, packet);

res._packet.confirmable = res._request.confirmable;
res._packet.token = res._request.token;
// end of work-around

const subscription = thing
.subscribeEvent(
thing.subscribeEvent(
segments[3],
// let subscription = event.subscribe(
(data) => {
Expand Down
2 changes: 1 addition & 1 deletion packages/binding-coap/src/coap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export class CoapOption {
}

export declare interface CoapRequestConfig {
agent?: Object;
agent?: Record<string, unknown>;
hostname?: string;
port?: number;
pathname?: string;
Expand Down
2 changes: 0 additions & 2 deletions packages/binding-coap/src/coaps-client-factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ import CoapsClient from "./coaps-client";
export default class CoapsClientFactory implements ProtocolClientFactory {
public readonly scheme: string = "coaps";

constructor(proxy?: string) {}

public getClient(): ProtocolClient {
console.debug("[binding-coap]", `CoapsClientFactory creating client for '${this.scheme}'`);
return new CoapsClient();
Expand Down
8 changes: 2 additions & 6 deletions packages/binding-coap/src/coaps-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,6 @@ export default class CoapsClient implements ProtocolClient {
// FIXME coap Agent closes socket when no messages in flight -> new socket with every request
private authorization: any;

constructor() {
// Intentionally blank
}

public toString(): string {
return "[CoapsClient]";
}
Expand Down Expand Up @@ -136,7 +132,7 @@ export default class CoapsClient implements ProtocolClient {
}

public setSecurity(metadata: Array<TD.SecurityScheme>, credentials?: any): boolean {
if (metadata === undefined || !Array.isArray(metadata) || metadata.length == 0) {
if (metadata === undefined || !Array.isArray(metadata) || metadata.length === 0) {
console.warn("[binding-coap]", `CoapsClient received empty security metadata`);
return false;
}
Expand Down Expand Up @@ -180,7 +176,7 @@ export default class CoapsClient implements ProtocolClient {

private generateRequest(form: CoapForm, dflt: string, content?: Content): any {
// url only works with http*
const requestUri = url.parse(form.href.replace(/$coaps/, "https"));
const requestUri = new URL(form.href.replace(/$coaps/, "https"));
coaps.setSecurityParams(requestUri.hostname, this.authorization);

let method: string = dflt;
Expand Down
22 changes: 10 additions & 12 deletions packages/binding-coap/test/coap-client-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@
import { suite, test } from "@testdeck/mocha";
import { expect, should } from "chai";

import { Helpers, ExposedThing } from "@node-wot/core";

import CoapServer from "../src/coap-server";
import CoapClient from "../src/coap-client";
// should must be called to augment all variables
Expand All @@ -30,15 +28,15 @@ should();
@suite("CoAP client implementation")
class CoapClientTest {
@test async "should apply form information"() {
const testThing = Helpers.extend(
{
name: "Test",
properties: {
test: {},
},
},
new ExposedThing(null)
);
// const testThing = Helpers.extend(
// {
// name: "Test",
// properties: {
// test: {},
// },
// },
// new ExposedThing(null)
// );
danielpeintner marked this conversation as resolved.
Show resolved Hide resolved
// testThing.extendInteractions();
// await testThing.writeProperty("test", "UNSET");

Expand Down Expand Up @@ -88,7 +86,7 @@ class CoapClientTest {
await coapServer.stop();
}

@test "should re-use port"(done: Function) {
@test "should re-use port"(done: () => {}) {
danielpeintner marked this conversation as resolved.
Show resolved Hide resolved
const coapServer = new CoapServer(56834, "localhost");
coapServer.start(null).then(() => {
const coapClient = new CoapClient(coapServer);
Expand Down
6 changes: 2 additions & 4 deletions packages/binding-file/src/file-client-factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,11 @@ import FileClient from "./file-client";
export default class FileClientFactory implements ProtocolClientFactory {
public readonly scheme: string = "file";

constructor(proxy?: string) {}

public getClient(): ProtocolClient {
console.debug("[binding-file]", `FileClientFactory creating client for '${this.scheme}'`);
return new FileClient();
}

public init = () => true;
public destroy = () => true;
public init = (): boolean => true;
public destroy = (): boolean => true;
}
18 changes: 8 additions & 10 deletions packages/binding-file/src/file-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,7 @@ import fs = require("fs");
import path = require("path");

export default class FileClient implements ProtocolClient {
constructor() {}

public toString() {
public toString(): string {
return "[FileClient]";
}

Expand Down Expand Up @@ -61,20 +59,20 @@ export default class FileClient implements ProtocolClient {
});
}

public writeResource(form: Form, content: Content): Promise<any> {
return new Promise<Object>((resolve, reject) => {
public writeResource(form: Form, content: Content): Promise<void> {
return new Promise<void>((resolve, reject) => {
reject(new Error(`FileClient does not implement write`));
});
}

public invokeResource(form: Form, payload: Object): Promise<any> {
return new Promise<Object>((resolve, reject) => {
public invokeResource(form: Form, content: Content): Promise<Content> {
return new Promise<Content>((resolve, reject) => {
reject(new Error(`FileClient does not implement invoke`));
});
}

public unlinkResource(form: Form): Promise<any> {
return new Promise<Object>((resolve, reject) => {
public unlinkResource(form: Form): Promise<void> {
return new Promise<void>((resolve, reject) => {
reject(new Error(`FileClient does not implement unlink`));
});
}
Expand All @@ -97,5 +95,5 @@ export default class FileClient implements ProtocolClient {
return true;
}

public setSecurity = (metadata: any) => false;
public setSecurity = (metadata: any): boolean => false;
}
10 changes: 5 additions & 5 deletions packages/binding-http/src/credential.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export class BasicCredential extends Credential {
this.password = password;
}

async sign(request: Request) {
async sign(request: Request): Promise<Request> {
const result = request.clone();
result.headers.set(
"authorization",
Expand All @@ -58,7 +58,7 @@ export class BearerCredential extends Credential {
this.token = token;
}

async sign(request: Request) {
async sign(request: Request): Promise<Request> {
const result = request.clone();
result.headers.set("authorization", "Bearer " + this.token);
return result;
Expand All @@ -79,7 +79,7 @@ export class BasicKeyCredential extends Credential {
this.options = options;
}

async sign(request: Request) {
async sign(request: Request): Promise<Request> {
const result = request.clone();

let headerName = "authorization";
Expand Down Expand Up @@ -108,7 +108,7 @@ export class OAuthCredential extends Credential {
this.token = token;
}

async sign(request: Request) {
async sign(request: Request): Promise<Request> {
if (this.token instanceof Promise) {
const tokenRequest = this.token as Promise<Token>;
this.token = await tokenRequest;
Expand All @@ -123,7 +123,7 @@ export class OAuthCredential extends Credential {
return mergeHeaders;
}

async refreshToken() {
async refreshToken(): Promise<OAuthCredential> {
if (this.token instanceof Promise) {
throw new Error("Uninitialized token. You have to call sing before refresh");
}
Expand Down
Loading