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

WIP: [ts-migration] lib/core #9870

Closed
wants to merge 8 commits into from
Closed

WIP: [ts-migration] lib/core #9870

wants to merge 8 commits into from

Conversation

kroeder
Copy link
Member

@kroeder kroeder commented Feb 16, 2020

Issue: #5030

@github-actions
Copy link
Contributor

github-actions bot commented Feb 16, 2020

Fails
🚫

PR is not labeled with one of: ["cleanup","doc-dependencies:update","BREAKING CHANGE","feature request","bug","documentation","maintenance","dependencies:update","dependencies","other"]

🚫 PR is marked with "do not merge", "in progress" labels.

Generated by 🚫 dangerJS against ca87ad4

@@ -1,9 +1,9 @@
export default class Provider {
getElements() {
getElements(type: string) {
Copy link
Member Author

Choose a reason for hiding this comment

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

fyi @ndelangen I had to modify the signature of getElements and handleAPI because every class that has extended this base class had these arguments implemented

I hope that's ok?

@kroeder
Copy link
Member Author

kroeder commented Feb 16, 2020

I found this in client/preview/start.ts

export const getContext = (() => {
  let cache;
  return decorateStory => {
    if (cache) {
      return cache;
    }

I think this is dead code; cache appears three times and is never passed to any method / function.

@ndelangen am I right?

there is an undefined check so it is allowed to be undefined
@ndelangen
Copy link
Member

@ndelangen am I right?

@kroeder It can likely be replaced with a memoize

@tmeasday
Copy link
Member

@kroeder - I am about to do a big of refactor of the preview files in core (src/client/preview/*).

I'll be typescript-ing the files I touch, hopefully I'll be done soon; perhaps in the meantime it'd be best to hold fire on those particular files in this PR? I'll bring across what you've done so far as a starting point.

@kroeder
Copy link
Member Author

kroeder commented Mar 17, 2020

@tmeasday what do I need to do to solve the conflicting files? git reset --soft on next? I think I want a merge of all my changes except those that you have merged into next, right?

@tmeasday
Copy link
Member

@kroeder that sounds about right, yeah.

# Conflicts:
#	lib/client-api/src/story_store.ts
#	lib/core/src/client/preview/start.tsx
#	lib/core/src/client/preview/types.ts
#	lib/core/src/client/preview/url.ts
@gaetanmaisse
Copy link
Member

Closing this one in favor of #12839 + another one coming soon to refine the types.

@stof stof deleted the ts-migration/core branch May 25, 2022 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants