Skip to content

Conversation

@ThibaudAV
Copy link
Contributor

@ThibaudAV ThibaudAV commented Jan 14, 2021

Issue: https://discord.com/channels/486522875931656193/490770949910691862/799325399635984465
maybe others 🤷‍♂️

What I did

  • Some small refactor
  • Fix : Detect a story template change and force the rendering

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.

@ThibaudAV ThibaudAV added angular cleanup Minor cleanup style change that won't show up in release changelog bug labels Jan 14, 2021
@ThibaudAV ThibaudAV marked this pull request as ready for review January 14, 2021 18:06
Comment on lines +8 to +10
export const moduleMetadata = (
metadata: Partial<NgModuleMetadata>
): DecoratorFunction<StoryFnAngularReturnType> => (storyFn) => {
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change too 🙈 however I'm not sure a lot of users should be impacted by it, maybe addons authors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the type before :
(storyFn: StoryFn<StoryFnAngularReturnType>) => any

the type after uses :

export declare type DecoratorFunction<StoryFnReturnType = unknown> = (fn: StoryFn<StoryFnReturnType>, c: StoryContext) => ReturnType<StoryFn<StoryFnReturnType>>;

type ReturnType<T extends (...args: any) => any> = T extends (...args: any) => infer R ? R : any;

The change is on any (it seems to me 🤔 ) =>
(storyFn: StoryFn<StoryFnAngularReturnType>) => StoryFnAngularReturnType

This will specify the return type but not change behavior
Indeed if the function is badly used it is likely to require a fix of type 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

My concern was more about the 2nd argument introduced by DecoratorFunction: c: StoryContext. The previous type implied only 1 arg: storyFn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:aa:
I forgot it. Indeed type imposes it. but it has always been present

Copy link
Member

Choose a reason for hiding this comment

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

Yeah... I'm really not sure it is a big deal as it's mainly internal plumbing

Copy link
Member

Choose a reason for hiding this comment

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

Indeed type imposes it. but it has always been present

So we can consider it as a fix more than a refactoring.

@gaetanmaisse gaetanmaisse removed the cleanup Minor cleanup style change that won't show up in release changelog label Jan 17, 2021
@ThibaudAV
Copy link
Contributor Author

@gaetanmaisse thank you for thinking about these aspects 🙏

What's your opinion on that @shilman @ndelangen?

I await your feedback and will make changes accordingly

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Looks good to me aside from the types issue. We can also make breaking changes to the types if needed, but it's probably more trouble than it's worth.

@ThibaudAV ThibaudAV force-pushed the angular-some-refactor branch from 9023d09 to 0be6bec Compare January 21, 2021 17:25
@ThibaudAV ThibaudAV requested a review from shilman January 21, 2021 18:09
Comment on lines +8 to +10
export const moduleMetadata = (
metadata: Partial<NgModuleMetadata>
): DecoratorFunction<StoryFnAngularReturnType> => (storyFn) => {
Copy link
Member

Choose a reason for hiding this comment

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

Indeed type imposes it. but it has always been present

So we can consider it as a fix more than a refactoring.

…gularReturnType

No longer appears to be used, based on a search on variable name
…shots

like app/angular/client/preview/types.ts
Preview/angular/* is now legacy. The decorators defined here are not. So they are moved to preview root
If the template is changed by a decorator, for example, it is necessary to re-render the story
@ThibaudAV ThibaudAV force-pushed the angular-some-refactor branch from 0be6bec to 920ad34 Compare January 21, 2021 19:46
@shilman shilman merged commit 240e7c1 into next Jan 22, 2021
@shilman shilman deleted the angular-some-refactor branch January 22, 2021 03:38
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.

4 participants