Some minor adjustments #12
Conversation
src/rebound/rebound.ts
Outdated
| } | ||
| } | ||
|
|
||
| //This is because a constructor should only be setting crucial variables. |
There was a problem hiding this comment.
can you please add proper JSDoc comments here
src/rebound/rebound.ts
Outdated
| @@ -158,4 +166,6 @@ export class Rebound { | |||
| * @method dispatch | |||
| */ | |||
| public dispatch: (name: ReboundEvent) => void = this._dispatch; | |||
|
|
|||
| public connect: () => void = this._connect; | |||
There was a problem hiding this comment.
Can you add the JSDoc comment here
src/rebound/rebound.ts
Outdated
| private _randId: string; | ||
| protected _randId: string; | ||
|
|
||
| protected _isHammerClient: boolean; |
There was a problem hiding this comment.
this is an unused variable... I would suggest adding the "no-unused-variable" property to tslint
There was a problem hiding this comment.
👕 Should remove this one as ClientType provides a way to tell HammerSpace that it's not a HammerSpace instance on the other side.
There was a problem hiding this comment.
Will remove.
| * @protected | ||
| * @method connect | ||
| */ | ||
| protected _connect() { |
There was a problem hiding this comment.
can you add tests for this please and thank you 😛
There was a problem hiding this comment.
Since we changed the constructor parameter the test cases need to be adjusted, will adjust them along with the unit tests for _connect
|
make a small refactor so the user will now create a rebound object like above. Thanks 🙂 |
…d as modules, creating a union of types so addEvents accepts a single string and an array decoupling constructor to only set crucial variables and creating a connect method so it can be overridden.
yanshcherbakovatredspace
force-pushed
the
feature/some-enhancements
branch
from
b23b68b to
c834840
Compare
src/client/client.spec.ts
Outdated
| @@ -1,28 +1,28 @@ | |||
| import { Client } from './client'; | |||
| import { Rebound } from '../rebound/rebound'; | |||
|
|
|||
| describe("Client:", () => { | |||
| describe('Client:', () => { | |||
| let client: any; | |||
There was a problem hiding this comment.
were you not able to remove this any too ?
There was a problem hiding this comment.
If I do then the tests will fail because in typescript you should not be able to access them.
src/client/client.spec.ts
Outdated
| let numOfKeysBefore = Object.keys(client._events).length; | ||
| let eventTypes = [1, [], {}, 1.2, undefined, null, true]; | ||
| let eventTypes = [1, [], {}, 1.2, undefined, undefined, true]; |
There was a problem hiding this comment.
this null was here as a test case for one of the different types of data that could be thrown at it
| expect(client._events.hasOwnProperty('testevent')).toBe(false); | ||
|
|
||
| client.addEvents('testevent'); | ||
| client.on('testevent', function() {}); | ||
| client.on('testevent', function() { | ||
| console.log('testevent'); |
There was a problem hiding this comment.
is this so it does not yell at you for having an empty function? because this console.log is not required as it only checking to see if the property is actually there and not what the data is
There was a problem hiding this comment.
yep just to remove a error
src/main.spec.ts
Outdated
| @@ -1,5 +1,5 @@ | |||
| describe("This test", function() { | |||
| it("will always pass", function() { | |||
| describe('This test', function() { | |||
There was a problem hiding this comment.
could you add a comment to this so people know why this is here and that it should not be removed
rebound.connect()