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

TypeScript migration: @storybook/channels #4977

merged 30 commits into from
Dec 20, 2018

Conversation

kroeder
Copy link
Member

@kroeder kroeder commented Dec 11, 2018

Issue: N/A

What I did

Migrated @storybook/channels to TypeScript

How to test

Tests are included - currently almost 100% coverage. Test for async: true is missing

@kroeder kroeder added ci: do not merge maintenance User-facing maintenance tasks typescript labels Dec 11, 2018
@kroeder kroeder self-assigned this Dec 11, 2018
lib/channels/src/index.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 11, 2018

Codecov Report

Merging #4977 into next will decrease coverage by 0.15%.
The diff coverage is 96.72%.

Impacted file tree graph

@@            Coverage Diff            @@
##             next   #4977      +/-   ##
=========================================
- Coverage   34.85%   34.7%   -0.16%     
=========================================
  Files         579     589      +10     
  Lines        7038    7262     +224     
  Branches      943     984      +41     
=========================================
+ Hits         2453    2520      +67     
- Misses       4082    4239     +157     
  Partials      503     503
Impacted Files Coverage Δ
lib/channels/src/index.ts 96.72% <96.72%> (ø)
app/angular/src/client/preview/render.js 0% <0%> (ø) ⬆️
...pp/angular/src/client/preview/angular-polyfills.js
...client/preview/angular/components/app.component.ts 27.02% <0%> (ø)
addons/notes/src/index.ts 84.61% <0%> (ø)
app/angular/src/demo/button.component.ts 0% <0%> (ø)
app/angular/src/demo/welcome.component.ts 0% <0%> (ø)
.../storyshots-core/src/frameworks/angular/helpers.ts 0% <0%> (ø)
...shots-core/src/frameworks/angular/app.component.ts 0% <0%> (ø)
...p/angular/src/client/preview/angular/decorators.ts 100% <0%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5b4e89...a92a3c5. Read the comment docs.

@kroeder kroeder requested a review from alterx as a code owner December 16, 2018 10:13
@kroeder
Copy link
Member Author

kroeder commented Dec 16, 2018

Rewrote all tests in order to reflect the actual usage of this package
Before, there were lots of tests on private class members

Coverage is almost 100% - I don't know how to test async: true (see todo comment in the code)

Also, the whole package is type safe - no any usage except for generics as default value (that makes them optional)

Copy link
Contributor

@Keraito Keraito left a comment

Choose a reason for hiding this comment

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

Awesome PR and work! Have some question regarding different aspects of TS and implementation. Would definitely like to help out with this! 😃

lib/channels/src/index.ts Outdated Show resolved Hide resolved
lib/channels/src/index.ts Outdated Show resolved Hide resolved
lib/channels/src/index.ts Show resolved Hide resolved

export interface ChannelEvent<TEventArgs = any> {
type: string; // eventName
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 😄 )

lib/channels/src/index.ts Outdated Show resolved Hide resolved
lib/channels/src/index.ts Outdated Show resolved Hide resolved
lib/channels/src/index.ts Show resolved Hide resolved
@ndelangen ndelangen self-assigned this Dec 17, 2018
@ndelangen
Copy link
Member

We need to NOT test this file:
examples/angular-cli/src/test

@ndelangen
Copy link
Member

ndelangen commented Dec 17, 2018

hmm

Number of tests 149 is 84% less than 913 in build #5452
[16:25:46]	Build failure condition: Anchor value for number of tests is taken from <latest successful build>. 
Fail build if number of tests is 20% less than the anchor value

and

ERR! FAILED to compile ts: @storybook/[email protected] 

@kroeder kroeder requested a review from saponifi3d as a code owner December 17, 2018 17:38
@ndelangen
Copy link
Member

Ready to merge?

@ndelangen ndelangen added this to the next milestone Dec 19, 2018
@kroeder
Copy link
Member Author

kroeder commented Dec 20, 2018

Ready to merge?

Yes, I still need feedback from you regarding the from property - see discussions!
Lets resolve this discussion, I might add a change and then we are ready to go!

@kroeder kroeder merged commit eb7071b into next Dec 20, 2018
@igor-dv igor-dv deleted the ts-migration/channels branch December 20, 2018 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants