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.

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

TypeScript migration: @storybook/channels #4977

Merged
merged 30 commits into from
Dec 20, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
237c594
WIP; Migrating @storybook/channels to TypeScript
kroeder Dec 11, 2018
176b650
WIP; Updated tests
kroeder Dec 11, 2018
6ae40ed
Added todo
kroeder Dec 11, 2018
f029b4c
Added missing export
kroeder Dec 11, 2018
766e576
Added lots of todos to tests and made them fail on purpose until they…
kroeder Dec 11, 2018
4008a90
Removed tests in order to write new ones that actually test the usage…
kroeder Dec 15, 2018
153fa26
Merge branch 'next' into ts-migration/channels
kroeder Dec 15, 2018
ec73e52
Working on tests and some smaller refactorings
kroeder Dec 15, 2018
22c2139
Support tests in TS
igor-dv Dec 16, 2018
f67bf03
Big refactoring + testing
kroeder Dec 16, 2018
f000c52
Resolved any
kroeder Dec 16, 2018
8db6baf
Refactoring
kroeder Dec 16, 2018
77b928e
Fixed jsnext:main
kroeder Dec 16, 2018
e47c348
Starting to migrate @storybook/channel-websocket
kroeder Dec 17, 2018
fa1d548
Added more types
kroeder Dec 17, 2018
bd543a8
Merge branch 'next' into ts-migration/channels
kroeder Dec 17, 2018
6ceb338
FIX CI by ignoring ng cli generated test file
ndelangen Dec 17, 2018
7fc0eb6
RENAME test.ts to karma.ts
ndelangen Dec 17, 2018
5815486
FIX CI by removing .spec. from jest testPattern
ndelangen Dec 17, 2018
1e860fe
Refactoring
kroeder Dec 17, 2018
fad2849
FIX missing tests, maybe
ndelangen Dec 17, 2018
a96a0b2
removed prepend and prependonce listener
kroeder Dec 17, 2018
e17a3d0
Merge remote-tracking branch 'origin/ts-migration/channels' into ts-m…
kroeder Dec 17, 2018
d328c27
Trying to fix CI error
kroeder Dec 19, 2018
068c118
Removed jsnext:main again
kroeder Dec 19, 2018
3f9579b
Reverted channel-websocket migration due to CI issues; the changes ar…
kroeder Dec 19, 2018
b49e201
Merge branch 'next' into ts-migration/channels
kroeder Dec 19, 2018
b0ded47
yarn.lock
kroeder Dec 19, 2018
708fdc7
Removed generics; they do not make sense - see PR discussion https://…
kroeder Dec 20, 2018
a92a3c5
This enum was wrong here, it should stick to string
kroeder Dec 20, 2018
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
29 changes: 21 additions & 8 deletions lib/channels/src/index.test.js → lib/channels/src/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
/* eslint no-underscore-dangle: 0 */
import Channel from '.';
import { Channel, ChannelEvent, ChannelTransport } from '.';

jest.useFakeTimers();

describe('Channel', () => {
let transport = null;
let channel = null;
let transport: ChannelTransport;
let channel: Channel;

beforeEach(() => {
transport = { setHandler: jest.fn(), send: jest.fn() };
Expand All @@ -18,18 +17,22 @@ describe('Channel', () => {
expect(transport.setHandler).toHaveBeenCalled();
});

// todo this needs to be tested differently; _transport is a private property
it('should not try to set handler if handler is missing', () => {
channel = new Channel();
expect(channel._transport).not.toBeDefined();
// expect(channel._transport).not.toBeDefined();
expect(true).toBe(false); // let it fail, until this is rewritten
});
});

// todo before, addListener was called with numbers; is this still the correct test?
describe('method:addListener', () => {
it('should call channel.on with args', () => {
const testFn = jest.fn();
channel.on = jest.fn();
channel.addListener(1, 2);
channel.addListener('A', testFn);
expect(channel.on).toHaveBeenCalled();
expect(channel.on).toHaveBeenCalledWith(1, 2);
expect(channel.on).toHaveBeenCalledWith('A', testFn);
});
});

Expand All @@ -43,13 +46,23 @@ describe('Channel', () => {
channel.emit(type, ...args);
expect(transport.send).toHaveBeenCalled();

const event = transport.send.mock.calls[0][0];
const event: ChannelEvent = transport.send.mock.calls[0][0];
expect(typeof event.from).toEqual('string');

delete event.from;
expect(event).toEqual(expected);
});

it('should be type safe', () => {
transport.send = jest.fn();
const type = 'test-type';
const args = [1, 2, 3];
const expected = { type, args };

// todo check if generic argument typing works
expect(true).toBe(false);
});

it('should call handle async option', () => {
transport.send = jest.fn();
const type = 'test-type';
Expand Down
80 changes: 54 additions & 26 deletions lib/channels/src/index.js → lib/channels/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,27 +1,60 @@
/* eslint no-underscore-dangle: 0 */
export interface ChannelTransport {
send: any; // todo Check actual type
setHandler<TEventArgs = any>(handler: (event: ChannelEvent<TEventArgs>) => void): void;
}

export interface ChannelEvent<TEventArgs = any> {
kroeder marked this conversation as resolved.
Show resolved Hide resolved
type: string;
from: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same goes for the from field similar to the type field, see comment above.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah should be either manager or preview

Copy link
Member Author

Choose a reason for hiding this comment

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

How can it be manager or preview if you check this

const event: ChannelEvent<TEventArgs[]> = { type: eventName, args, from: this.sender };

Where this.sender is

  private sender = generateRandomId();

and the function does

const generateRandomId = () => {
  // generates a random 13 character string
  return Math.random()
    .toString(16)
    .slice(2);
};

Either we rewrite some stuff or I can't do this:

  from: 'preview' | 'manager';

due to type checking errors (all hail typescript 😄 )

args: TEventArgs[];
}

export interface Listener<TEventArgs = any> {
(...args: TEventArgs[]): void;
ignorePeer?: boolean;
}

interface ListenersKeyValue {
[key: string]: Listener[];
kroeder marked this conversation as resolved.
Show resolved Hide resolved
}

export default class Channel {
constructor({ transport, async } = {}) {
this._sender = this._randomId();
interface ChannelArgs {
transport?: ChannelTransport;
async?: boolean;
}

const generateRandomId = () => {
// generates a random 13 character string
return Math.random()
.toString(16)
.slice(2);
};

export class Channel {
private _sender = generateRandomId();
private _listeners: ListenersKeyValue = {};
private readonly _async: boolean = false;
kroeder marked this conversation as resolved.
Show resolved Hide resolved
private readonly _transport: ChannelTransport;

constructor({ transport, async }: ChannelArgs = {}) {
this._async = async;
if (transport) {
this._transport = transport;
this._transport.setHandler(event => this._handleEvent(event));
}
this._listeners = {};
}

addListener(type, listener) {
addListener(type: string, listener: Listener) {
this.on(type, listener);
}

addPeerListener(type, listener) {
addPeerListener(type: string, listener: Listener) {
const peerListener = listener;
peerListener.ignorePeer = true;
this.on(type, peerListener);
}

emit(type, ...args) {
emit<TEventArgs = any>(type: string, ...args: TEventArgs[]) {
const event = { type, args, from: this._sender };

const handler = () => {
Expand All @@ -42,69 +75,64 @@ export default class Channel {
return Object.keys(this._listeners);
}

listenerCount(type) {
listenerCount(type: string) {
const listeners = this._listeners[type];
return listeners ? listeners.length : 0;
}

listeners(type) {
listeners(type: string) {
return this._listeners[type];
}

on(type, listener) {
on(type: string, listener: Listener) {
this._listeners[type] = this._listeners[type] || [];
this._listeners[type].push(listener);
}

once(type, listener) {
once(type: string, listener: Listener) {
const onceListener = this._onceListener(type, listener);
this.on(type, onceListener);
}

prependListener(type, listener) {
prependListener(type: string, listener: Listener) {
this._listeners[type] = this._listeners[type] || [];
this._listeners[type].unshift(listener);
}

prependOnceListener(type, listener) {
prependOnceListener(type: string, listener: Listener) {
const onceListener = this._onceListener(type, listener);
this.prependListener(type, onceListener);
}

removeAllListeners(type) {
removeAllListeners(type: string) {
if (!type) {
this._listeners = {};
} else if (this._listeners[type]) {
delete this._listeners[type];
}
}

removeListener(type, listener) {
removeListener(type: string, listener: Listener) {
const listeners = this._listeners[type];
if (listeners) {
this._listeners[type] = listeners.filter(l => l !== listener);
}
}

_randomId() {
// generates a random 13 character string
return Math.random()
.toString(16)
.slice(2);
}

_handleEvent(event, isPeer) {
private _handleEvent(event: ChannelEvent, isPeer = false) {
const listeners = this._listeners[event.type];
if (listeners && (isPeer || event.from !== this._sender)) {
listeners.forEach(fn => !(isPeer && fn.ignorePeer) && fn(...event.args));
}
}

_onceListener(type, listener) {
const onceListener = (...args) => {
private _onceListener(type: string, listener: Listener) {
const onceListener = (...args: any[]) => {
this.removeListener(type, onceListener);
return listener(...args);
};
return onceListener;
}
}

export default Channel;
13 changes: 13 additions & 0 deletions lib/channels/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"extends": "../../tsconfig.json",
"compilerOptions": {
"rootDir": "./src"
},
"include": [
"src/**/*.ts",
"src/**/*.tsx"
],
"exclude": [
"src/index.test.ts"
]
}