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

Ts migration/client api #7147

Merged
merged 16 commits into from
Jul 6, 2019
Merged

Ts migration/client api #7147

merged 16 commits into from
Jul 6, 2019

Conversation

Jessica-Koch
Copy link
Contributor

Issue:

What I did

migrated the client-api to typescript, there are some errors, I wasn't sure how to type a couple of functions so I left them as is. Everything builds.

How to test

  • Is this testable with Jest or Chromatic screenshots? yes
  • Does this need a new example in the kitchen sink apps? no
  • Does this need an update to the documentation? no

If your answer is yes to any of these, please make sure to include it in your PR.

@vercel
Copy link

vercel bot commented Jun 19, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-ts-migration-client-api.storybook.now.sh

export default class ClientApi {
constructor({ storyStore, decorateStory = defaultDecorateStory } = {}) {
_storyStore: StoryStore;
Copy link
Member

Choose a reason for hiding this comment

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

could we use private fields here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes we can.

@ndelangen
Copy link
Member

src/client_api.ts(36,32): error TS7006: Parameter 'context' implicitly has an 'any' type.
src/client_api.ts(38,10): error TS7006: Parameter 'p' implicitly has an 'any' type.
src/client_api.ts(165,7): error TS7017: Element implicitly has an 'any' type because type '{ kind: string; }' has no index signature.
src/client_api.ts(166,15): error TS2339: Property 'apply' does not exist on type 'Addon'.
src/client_api.ts(171,9): error TS2339: Property 'add' does not exist on type '{ kind: string; }'.
src/client_api.ts(208,49): error TS2345: Argument of type '{}' is not assignable to parameter of type 'IHierarchyObj'.
  Type '{}' is missing the following properties from type 'IHierarchyObj': hierarchyRootSeparator, hierarchySeparator
src/client_api.ts(227,9): error TS2554: Expected 1 arguments, but got 2.
src/client_api.ts(240,9): error TS2339: Property 'addDecorator' does not exist on type '{ kind: string; }'.
src/client_api.ts(251,9): error TS2339: Property 'addParameters' does not exist on type '{ kind: string; }'.
src/story_store.ts(102,21): error TS7031: Binding element 'storyId' implicitly has an 'any' type.
src/story_store.ts(102,30): error TS7031: Binding element 'viewMode' implicitly has an 'any' type.
src/story_store.ts(158,43): error TS2322: Type 'void' is not assignable to type '{}'.
src/story_store.ts(214,30): error TS2339: Property 'fileName' does not exist on type '{} | { fileName: string; }'.
  Property 'fileName' does not exist on type '{}'.
src/types.ts(347,12): error TS2304: Cannot find name 'storyFn'.
src/types.ts(347,12): error TS4033: Property 'storyFn' of exported interface has or is using private name 'storyFn'.

@Jessica-Koch
Copy link
Contributor Author

that's odd, I resolved those, will go back and check again.

@@ -14,10 +48,17 @@ export default class ConfigApi {

_renderMain() {
// do initial render of story
debugger;
Copy link
Member

Choose a reason for hiding this comment

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

hmm not sure it should be still here 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it should not, i am actually battling with node-gyp and cannot build storybook anymore, will be updating PR HOPEFULLY tonight; post reformating

Jessica Koch and others added 3 commits June 30, 2019 14:47
@Jessica-Koch Jessica-Koch requested a review from theinterned as a code owner July 1, 2019 07:33
@vercel vercel bot temporarily deployed to staging July 1, 2019 07:33 Inactive
@ndelangen ndelangen force-pushed the ts-migration/client-api branch from 76239e9 to 8eb8ffb Compare July 1, 2019 14:27
@ndelangen ndelangen force-pushed the ts-migration/client-api branch from 8eb8ffb to bcf18cc Compare July 2, 2019 00:12
@vercel vercel bot temporarily deployed to staging July 2, 2019 00:12 Inactive
@ndelangen
Copy link
Member

@gaetanmaisse could you possibly help review this?

parameters: {
[key: string]: any;
};
}

export interface OptionsParameter extends Object {
Copy link
Member

Choose a reason for hiding this comment

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

Just wonder why extends Object?

lib/client-api/src/client_api.ts Outdated Show resolved Hide resolved
lib/client-api/src/client_api.ts Show resolved Hide resolved
lib/client-api/src/config_api.ts Show resolved Hide resolved
lib/client-api/src/story_store.ts Show resolved Hide resolved
lib/client-api/src/story_store.ts Show resolved Hide resolved
@ndelangen
Copy link
Member

Thank you for the review @gaetanmaisse


_data: StoreData;

_legacyData?: LegacyData;
Copy link
Member

@lonyele lonyele Jul 3, 2019

Choose a reason for hiding this comment

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

My guess is that you were trying to fix naming from _legacydata to _legacyData but somehow not finished or discarded changing?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fearful of changing too much, this SHOULd be an internal api, but I am aware of some integrations hooking into this, and preferably I would like to change it at a later point at the 6.0 release instead.

Copy link
Member

Choose a reason for hiding this comment

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

@ndelangen what's _legacyData?! Seems unused throughout the codebase?

# Conflicts:
#	lib/client-api/package.json
ndelangen added 2 commits July 6, 2019 14:13
# Conflicts:
#	addons/ondevice-backgrounds/src/BackgroundPanel.js
#	lib/client-api/package.json
#	lib/client-api/src/story_store.ts
@ndelangen ndelangen merged commit 0083afd into next Jul 6, 2019
@ndelangen ndelangen deleted the ts-migration/client-api branch July 6, 2019 13:29
shilman added a commit that referenced this pull request Jul 8, 2019
input.replace(/[^a-z0-9]+([a-z0-9])/gi, (...params) => params[1].toUpperCase());

const toChild = it => ({ ...it });
const toChild = (it: {}) => ({ ...it });
Copy link
Member

Choose a reason for hiding this comment

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

@ndelangen unused?

Copy link
Member

Choose a reason for hiding this comment

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

yes

@@ -130,26 +144,25 @@ export default class ClientApi {
if (m && m.hot && m.hot.dispose) {
m.hot.dispose(() => {
const { _storyStore } = this;
_storyStore.remove();
Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks preview-api typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants