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

Addons and app changes for on demand store #16010

Closed

Conversation

tmeasday
Copy link
Member

Part of #15871

Telescoping on #16009

What I did

Updated addons + apps for on-demand store. Mostly updating types

@tmeasday
Copy link
Member Author

@ThibaudAV there were a few more changes to Angular that you might want to take a look at.

@nx-cloud
Copy link

nx-cloud bot commented Sep 10, 2021

Nx Cloud Report

We didn't find any information for the current pull request with the commit e29268c.
You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.


Sent with 💌 from NxCloud.

@github-actions
Copy link
Contributor

Fails
🚫

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

Generated by 🚫 dangerJS against e29268c

@tmeasday tmeasday mentioned this pull request Sep 10, 2021
14 tasks
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.

Looking good! 🚀

forceRender,
}: RenderContext) {
const Story = unboundStoryFn as FunctionComponent<StoryContext>;
export async function renderToDOM(
Copy link
Member

Choose a reason for hiding this comment

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

let's convert these to named exports 🙏

Copy link
Member Author

Choose a reason for hiding this comment

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

That's coming

Copy link
Member Author

Choose a reason for hiding this comment

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

app/ember/src/client/preview/render.ts Show resolved Hide resolved
app/html/src/client/preview/render.ts Show resolved Hide resolved
app/preact/src/client/preview/render.tsx Show resolved Hide resolved
app/svelte/src/client/preview/render.ts Show resolved Hide resolved
app/vue/src/client/preview/render.ts Show resolved Hide resolved
app/vue3/src/client/preview/render.ts Show resolved Hide resolved
app/angular/src/client/preview/render.ts Show resolved Hide resolved

export type Parameters = DefaultParameters & {
/** Uses legacy angular rendering engine that use dynamic component */
angularLegacyRendering?: boolean;
component: unknown;
Copy link
Contributor

Choose a reason for hiding this comment

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

why delete this type?

it allows to force the component of a story if the one defined in export default is not the one wanted
ex :

Copy link
Contributor

Choose a reason for hiding this comment

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

it seemed to me that the component property in the story should be deprecated 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

@shilman perhaps you care to respond to this one -- the rationale is that this method of overriding the component was an implementation detail, and can be considered a bug.

@@ -45,7 +44,7 @@ export const getStorybookModuleMetadata = (
if (storyComponent) {
deprecatedStoryComponentWarning();
}
const component = storyComponent ?? parameters.component;
const component = storyComponent ?? annotatedComponent;
Copy link
Contributor

Choose a reason for hiding this comment

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

this will be create braking change. i think

because before, to have a specific component for a story, you had to pass it through the parameters

Now if I understand correctly we can make a :

        export const Primary: Story = () => ({});
        Primary.component = AppComponent;

maybe add a message in the MIGRATION.md

Copy link
Contributor

@ThibaudAV ThibaudAV left a comment

Choose a reason for hiding this comment

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

a big job 👏 👏 👏 .
there are a lot of changes. which version is it for?

@tmeasday
Copy link
Member Author

@ThibaudAV

there are a lot of changes. which version is it for?

It's for 6.4. There aren't intended to be any functional changes for the angular layer, mostly it is just due to updating types. Do you see any other changes that concern you (barring the parameters.component part)?.

@shilman
Copy link
Member

shilman commented Sep 11, 2021

@ThibaudAV @tmeasday

Here is my understanding of the situation:

  • In 6.1, the user could specify () => ({ component: SomeComponent })
  • In 6.2, @ThibaudAV refactored so that:
    • The story return value's component was deprecated
    • It would use default.component if there was no component in the story return value
    • @ThibaudAV used parameters.component to override default.component
  • Now, in 6.4 @tmeasday 's refactor:
    • Removes parameters.component. It was an implementation detail that was never intended as a public API.
    • Rewrites the examples to use the deprecated component in the story return value

Given all that:

Assuming it's OK to restrict @storybook/angular users to a single component per file, we can tell them to use the deprecated story return value's component until 7.0 and then we can remove it as a breaking change in 7.0.

If that is too restrictive, we can un-deprecate it. Or we can discuss other options.

@ThibaudAV Please let me know if that all makes sense. I don't understand enough about the Angular use case, so let's please discuss here! 🙏

targetDOMNode,
}: {
storyFnAngular: StoryFnAngularReturnType;
forced: boolean;
component?: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would tend to say that the correct ts type for the reference to an angular component class is :
Type

I'm not sure of myself. but it can avoid any and show that we are referring to an angular component
🙃

mainStoryFn: StoryFn<StoryFnAngularReturnType>,
decorators: DecoratorFunction<StoryFnAngularReturnType>[]
): StoryFn<StoryFnAngularReturnType> {
mainStoryFn: LegacyStoryFn<AngularFramework>,
Copy link
Contributor

@ThibaudAV ThibaudAV Sep 11, 2021

Choose a reason for hiding this comment

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

how to get rid of the LegacyStoryFn ? ( in the future )

it's not the same type as ProjectAnnotations<...>['applyDecorators']; or PartialStoryFn<AngularFramework>,

only for my personal understanding 😄 🙏

@ThibaudAV
Copy link
Contributor

@shilman Ok. Thank you for the summary

Assuming it's OK to restrict @storybook/angular users to a single component per file, we can tell them to use the deprecated story return value's component until 7.0 and then we can remove it as a breaking change in 7.0.

If that is too restrictive, we can un-deprecate it. Or we can discuss other options.

I think it's good, it forces to have coherent stories. 🤷‍♂️
But I've seen some people use this possibility to make a story with several similar components, or for stories about the "wrapper/parent" component.
for which the only solution is to use template property (if component dose not exist)

I'm thinking of some improvements on this
like being able to use the template created by default from the "export default.component" in each story.
-> allows to create a story about a component in different parent html context

or to be able to add to this same template only an inner template (<c>innerTemplate</c>) without redefining all the template
-> (oppose it) . Allow to have a story on a component with different inner html context


@tmeasday

Do you see any other changes that concern you (barring the parameters.component part)?.

Nope 🚀
If you need help fixing examples or tests for angular let me know

@shilman
Copy link
Member

shilman commented Sep 11, 2021

Cc @kroeder @Marklb 👆

@shilman
Copy link
Member

shilman commented Sep 14, 2021

Merged as part of #15871

@shilman shilman closed this Sep 14, 2021
@TAharshalcarpenter
Copy link

Nope 🚀 If you need help fixing examples or tests for angular let me know

@ThibaudAV Can you please help me on how to use @storybook/addon-controls instead of @storybook/addon-knobs in Angular ?

I am only finding examples of usage in React ..

@manuelmeister
Copy link
Contributor

@shilman I think the codemod for storiesOf still transforms the stories to parameters: { component: SomeComponent }

@shilman
Copy link
Member

shilman commented Jan 7, 2022

Good call @manuelmeister opening an issue for that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants