Angular: Deprecate the story component attribute #13383

ThibaudAV · 2020-12-05T11:22:33Z

Issue: #8673

TODO :

Add MIGRATION.md info
change the current examples
check the doc

What I did

use same "angular preview render" in storyshots and in app/angular
fix some exemples
deprecate story component attribute
update all exemples

How to test

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

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

app/angular/src/client/preview/angular-beta/RenderNgAppService.ts

ThibaudAV · 2020-12-08T18:10:41Z

@shilman I'm not sure what to put in MIGRATION.md 🤷‍♂️ Can I let you do it?

some comments :

It's a mess in angular-cli examples ^^
I did my best for the doc snippets. but I think it needs another look

ThibaudAV · 2020-12-08T22:41:48Z

addons/storyshots/storyshots-core/src/frameworks/angular/renderTree.ts

@@ -7,35 +7,35 @@ import { BrowserDynamicTestingModule } from '@angular/platform-browser-dynamic/t
 // eslint-disable-next-line import/no-extraneous-dependencies
 import { NO_ERRORS_SCHEMA } from '@angular/core';
 import { addSerializer } from 'jest-specific-snapshot';
-import { initModuleData } from './helpers';
+import { RenderNgAppService } from '@storybook/angular/dist/client/preview/angular-beta/RenderNgAppService';


Avoids redefining the rendering like before.
if you have a (simple) idea to avoid the /dist/ tell me :)

shilman

This is so awesome @ThibaudAV 💯 Thanks for pushing it forward.

I wrote a MIGRATION and added a couple comments to the PR including one change request.

Would be great to get more @storybookjs/angular eyes on it to critique from an Angular standpoint. From a CSF standpoint it looks like a big improvement to me. I also appreciate the example cleanup.

One more thing, can we leave around an "old-style" story as a test case, if there's not one already? Or create one?

app/angular/src/client/preview/angular-beta/RenderNgAppService.ts

docs/snippets/angular/button-story-component-decorator.ts.mdx

ignore class like doc-injectable example

…ngular examples

…ngular docs

…ngular cli

shilman · 2020-12-10T14:54:38Z

@ThibaudAV I think we can sneak in a breaking change if that's necessary and force users to regenerate their snapshots. But I don't understand why that's needed and would certainly prefer to avoid it if it's not too much work.

ThibaudAV · 2020-12-11T00:03:16Z

@shilman
IMHO: I think it would be better to change the snapshots. Otherwise, there will be other problems

To generate the snapshot storyshot needs some information built by the "render".
Before there was a code duplication and so 2 versions of the render (one for addon/storyshot and one for storybook/angular)
This is problematic because the storyshot version does not benefit from the improvements and corrections of the main part (storybook/angular), for some time now
It also makes tests that do not reflect precisely the generated stories.

If we continue with the same code as before the new internal storybook functionality will not work with storyshot anymore. and this will make the storyshot completely legacy. :/

To prevent snapshots from changing due to internal storybook modifications🤞
My last commit removes from snapshots some internal properties that may change in the future.
And allows snapshots to contain mainly the story code.

What do you think about adding also a message in MIGRATION to inform that storyshot is improving to contain fewer internal storybook properties? And that it will be necessary to regenerate them 🙈 but it's for a good reason

I have a little talk with @gaetanmaisse who will surely be able to complete some information

I don't know if it's reassuring or not but when angular or angular-material changes minor version and you have test snapshot on your project it's not uncommon to have to regenerate them.

In any case, I thank you for accompanying me in these changes, it is very nice to have your opinion. 😌

Generates snapshots that will change less if the internal stroybook properties change

gaetanmaisse · 2020-12-13T15:13:36Z

I think the final result we have for story snapshots is a huge improvement because:

it now really matches what is rendered when running Storybook
all Storybook renderer specific properties aren't included in the snapshots anymore, so we can now update the renderer internal plumbing without causing any update of the snapshots 👌🏻

So the remaining question is: are we ok to ask SB users to update all their snapshots when updating to 6.2? Or must we wait for 7.0 to do any change in @storybook/addon-storyshot?

I would prefer to merge and release this in 6.2 as it's part of major improvements of SB for Angular 🤷🏻

gaetanmaisse

The code LGTM, if we agree on making this land in 6.2 we have to add a section about snapshots update in MIGRATION.md

shilman · 2020-12-14T02:42:18Z

@ThibaudAV @gaetanmaisse thanks for getting this over the line. I've updated the MIGRATION instructions as I understand them:

We've updated the Angular storyshots format in 6.2, which is technically a breaking change. Apologies to semver purists: if you're using storyshots, you'll need to update your snapshots. The new format hides the implementation details of @storybook/angular so that we can evolve its renderer without breaking your snapshots in the future.

I'm OK with this compromise, but since this is a breaking change in a minor release I'm going to give the rest of the maintainers a day or two to chime in before merging.

MIGRATION.md

Co-authored-by: Manuel Meister <[email protected]>

# Conflicts: # MIGRATION.md

ThibaudAV · 2020-12-15T17:09:53Z

app/angular/src/client/index.ts

@@ -9,6 +9,8 @@ export {
  raw,
 } from './preview';

+export { RenderNgAppService } from './preview/angular-beta/RenderNgAppService';


I hadn't dared to export this service in the public index. ^^

I'm not sure this is the right way. However, we recently introduced ESM into the codebase and it broke things: #13013. If there's a less aggressive way to do it, I'm also open to that!

shilman · 2020-12-16T03:00:45Z

addons/storyshots/storyshots-core/src/frameworks/angular/renderTree.ts

@@ -5,7 +5,7 @@ import { TestBed } from '@angular/core/testing';
 // eslint-disable-next-line import/no-extraneous-dependencies
 import { BrowserDynamicTestingModule } from '@angular/platform-browser-dynamic/testing';
 import { addSerializer } from 'jest-specific-snapshot';
-import { RenderNgAppService } from '@storybook/angular';
+import { RenderNgAppService } from '@storybook/angular/renderer';


@ThibaudAV trying this as something less aggressive 😄

yes but I can see that it doesn't seem simple 💪 . Tell me if you need help with this PR

shilman · 2020-12-16T11:40:50Z

@ThibaudAV I think this is ready to merge. WDYT?

ThibaudAV · 2020-12-16T12:59:54Z

@shilman 🤞 yes

shilman · 2021-01-04T10:12:18Z

cc @mandarini

ThibaudAV changed the title ~~feat(angular): depreciate the story component attribute~~ Angular: depreciate the story component attribute Dec 5, 2020

ThibaudAV mentioned this pull request Dec 5, 2020

Angular: Use parameters.component if available #8673

Closed

shilman requested a review from a team December 5, 2020 13:29

shilman added angular maintenance User-facing maintenance tasks labels Dec 5, 2020

ThibaudAV force-pushed the angular-parameters-component branch from 70a8166 to 5210e22 Compare December 8, 2020 17:55

ThibaudAV commented Dec 8, 2020

View reviewed changes

app/angular/src/client/preview/angular-beta/RenderNgAppService.ts Outdated Show resolved Hide resolved

ThibaudAV force-pushed the angular-parameters-component branch 4 times, most recently from 499b74e to a6a833a Compare December 8, 2020 22:41

ThibaudAV commented Dec 8, 2020

View reviewed changes

ThibaudAV force-pushed the angular-parameters-component branch from a6a833a to a28d590 Compare December 8, 2020 22:52

shilman changed the title ~~Angular: depreciate the story component attribute~~ Angular: deprecate the story component attribute Dec 9, 2020

shilman changed the title ~~Angular: deprecate the story component attribute~~ Angular: Deprecate the story component attribute Dec 9, 2020

shilman requested changes Dec 9, 2020

View reviewed changes

app/angular/src/client/preview/angular-beta/RenderNgAppService.ts Outdated Show resolved Hide resolved

docs/snippets/angular/button-story-component-decorator.ts.mdx Show resolved Hide resolved

shilman added this to the 6.2 core milestone Dec 9, 2020

ThibaudAV added 6 commits December 9, 2020 17:05

fix(angular): declare only the compatible component

c7dc891

ignore class like doc-injectable example

fix(angular): use @storybook/angular client rendering in storyshots

acd8104

feat(angular): depreciate the story component attribute

6e6de1b

refactor(angular): take into account the component deprecation in a…

05848fe

…ngular examples

refactor(angular): take into account the component deprecation in a…

1b65c45

…ngular docs

refactor(angular): take into account the component deprecation in a…

0f55784

…ngular cli

ThibaudAV force-pushed the angular-parameters-component branch from 08e8a7a to 0f55784 Compare December 9, 2020 16:27

shilman added 2 commits December 9, 2020 17:42

MIGRATION: Deprecate Angular story component

e3dbfa2

Fix link

1fe60ca

ThibaudAV marked this pull request as ready for review December 9, 2020 17:03

ThibaudAV requested review from alterx and igor-dv as code owners December 9, 2020 17:03

ThibaudAV added 2 commits December 11, 2020 01:07

fixup! feat(angular): depreciate the story component attribute

8b97b77

feat(angular): remove internal storybook properties in storyshot

901759a

Generates snapshots that will change less if the internal stroybook properties change

ThibaudAV force-pushed the angular-parameters-component branch from bf3a014 to 901759a Compare December 11, 2020 00:07

ThibaudAV requested review from shilman and gaetanmaisse December 11, 2020 00:08

gaetanmaisse approved these changes Dec 13, 2020

View reviewed changes

shilman added 2 commits December 14, 2020 10:35

Document Angular snapshot migration

9f92644

Merge branch 'next' into pr/13383

e3b6d28

manuelmeister reviewed Dec 14, 2020

View reviewed changes

MIGRATION.md Outdated Show resolved Hide resolved

shilman and others added 2 commits December 14, 2020 23:30

Update MIGRATION.md

4f87404

Co-authored-by: Manuel Meister <[email protected]>

Merge branch 'next' into angular-parameters-component

9048df8

# Conflicts: # MIGRATION.md

shilman added the run e2e extended test suite Run the e2e extended test suite in CircleCI workflows label Dec 15, 2020

shilman added 2 commits December 15, 2020 23:28

Merge branch 'next' into pr/13383

be939de

Fix angular storyshots

1dec7c6

ThibaudAV commented Dec 15, 2020

View reviewed changes

shilman added 2 commits December 16, 2020 10:49

Merge branch 'next' into pr/13383

2a7c9f6

Angular: Special renderer entry for storyshots

d2bd6e5

shilman reviewed Dec 16, 2020

View reviewed changes

shilman added 3 commits December 16, 2020 11:10

Angular: Fix renderer export

761aad4

Update renderer.js

67d0314

Update renderer.js

6149b14

shilman merged commit 518f8ec into storybookjs:next Dec 16, 2020

shilman mentioned this pull request Jan 28, 2021

Chore: Adds missing Angular snippets #13749

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Angular: Deprecate the story component attribute #13383

Angular: Deprecate the story component attribute #13383

ThibaudAV commented Dec 5, 2020 •

edited by shilman

Loading

ThibaudAV commented Dec 8, 2020

ThibaudAV Dec 8, 2020

shilman left a comment

shilman commented Dec 10, 2020 •

edited

Loading

ThibaudAV commented Dec 11, 2020 •

edited by gaetanmaisse

Loading

gaetanmaisse commented Dec 13, 2020

gaetanmaisse left a comment

shilman commented Dec 14, 2020

ThibaudAV Dec 15, 2020

shilman Dec 16, 2020

shilman Dec 16, 2020

ThibaudAV Dec 16, 2020

shilman commented Dec 16, 2020

ThibaudAV commented Dec 16, 2020 •

edited

Loading

shilman commented Jan 4, 2021

Angular: Deprecate the story component attribute #13383

Angular: Deprecate the story component attribute #13383

Conversation

ThibaudAV commented Dec 5, 2020 • edited by shilman Loading

What I did

How to test

ThibaudAV commented Dec 8, 2020

ThibaudAV Dec 8, 2020

Choose a reason for hiding this comment

shilman left a comment

Choose a reason for hiding this comment

shilman commented Dec 10, 2020 • edited Loading

ThibaudAV commented Dec 11, 2020 • edited by gaetanmaisse Loading

gaetanmaisse commented Dec 13, 2020

gaetanmaisse left a comment

Choose a reason for hiding this comment

shilman commented Dec 14, 2020

ThibaudAV Dec 15, 2020

Choose a reason for hiding this comment

shilman Dec 16, 2020

Choose a reason for hiding this comment

shilman Dec 16, 2020

Choose a reason for hiding this comment

ThibaudAV Dec 16, 2020

Choose a reason for hiding this comment

shilman commented Dec 16, 2020

ThibaudAV commented Dec 16, 2020 • edited Loading

shilman commented Jan 4, 2021

ThibaudAV commented Dec 5, 2020 •

edited by shilman

Loading

shilman commented Dec 10, 2020 •

edited

Loading

ThibaudAV commented Dec 11, 2020 •

edited by gaetanmaisse

Loading

ThibaudAV commented Dec 16, 2020 •

edited

Loading