Angular: Component creation with TestBed API#31311
Conversation
feat: add provider story in angular storybook template
refactor: set wrapper component in render function refactor: wrapper creation refactor: metadata override on TestBedComponentBuilder.ts
refactor: init testbed on creation
There was a problem hiding this comment.
7 file(s) reviewed, 14 comment(s)
Edit PR Review Bot Settings | Greptile
|
|
||
| const applicationRefs = new Map<HTMLElement, ApplicationRef>(); | ||
|
|
||
| const componentBuilders: TestBedComponentBuilder[] = []; |
There was a problem hiding this comment.
logic: Memory leak potential - componentBuilders array grows but never cleared
| setStoryFn(storyFn: StoryFnAngularReturnType) { | ||
| this.styles = storyFn.styles; | ||
| this.schemas = storyFn.moduleMetadata?.schemas; | ||
| console.log(this.schemas); |
There was a problem hiding this comment.
style: Remove debug console.log statement before merging
| console.log(this.schemas); |
| if (this.props != null) | ||
| this.fixture.componentInstance = Object.assign(this.fixture.componentInstance, this.props); | ||
| this.fixture.detectChanges(); |
There was a problem hiding this comment.
logic: Potential race condition if updateComponentProps is called before fixture is initialized. Add null check for this.fixture
| if (this.props != null) | |
| this.fixture.componentInstance = Object.assign(this.fixture.componentInstance, this.props); | |
| this.fixture.detectChanges(); | |
| if (this.props != null && this.fixture) { | |
| this.fixture.componentInstance = Object.assign(this.fixture.componentInstance, this.props); | |
| this.fixture.detectChanges(); | |
| } |
|
|
||
| private component: Type<unknown> | undefined; | ||
|
|
||
| private fixture: ComponentFixture<unknown>; |
There was a problem hiding this comment.
logic: fixture is used before initialization - should be marked as optional with '?' or initialized in constructor
| @Component({ | ||
| selector, | ||
| template, | ||
| standalone: true, | ||
| imports: [StorybookComponentModule], | ||
| providers, | ||
| styles, | ||
| schemas: moduleMetadata.schemas, | ||
| }) |
There was a problem hiding this comment.
logic: Missing selector property in @component decorator could cause issues with component instantiation
| ) => { | ||
| return { | ||
| set: { | ||
| exports: [...declarations, ...imports], |
There was a problem hiding this comment.
logic: Spreading both declarations and imports into exports could cause naming conflicts if components have the same selectors
| this.label = apiService.data; | ||
| } | ||
|
|
||
| label = 'NotSetYet'; |
There was a problem hiding this comment.
logic: label property initialized after constructor runs - move initialization to constructor to avoid potential undefined state
| label = 'NotSetYet'; | |
| label: string; |
| constructor(private apiService: ApiService) { | ||
| this.label = apiService.data; | ||
| } |
There was a problem hiding this comment.
style: consider making apiService readonly since it's only used in constructor
There was a problem hiding this comment.
7 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| this.testBedInstance = new TestBed(); | ||
| this.testBedInstance.initTestEnvironment( | ||
| BrowserDynamicTestingModule, | ||
| platformBrowserDynamicTesting() | ||
| ); |
There was a problem hiding this comment.
logic: TestBed environment is initialized in constructor which could cause issues if multiple instances are created. Consider moving to a static initialization or adding checks to prevent multiple initializations.
| this.testBedInstance = new TestBed(); | |
| this.testBedInstance.initTestEnvironment( | |
| BrowserDynamicTestingModule, | |
| platformBrowserDynamicTesting() | |
| ); | |
| if (!TestBed.initialized) { | |
| TestBed.initTestEnvironment( | |
| BrowserDynamicTestingModule, | |
| platformBrowserDynamicTesting() | |
| ); | |
| } | |
| this.testBedInstance = TestBed; |
| exports: [...declarations, ...imports], | ||
| declarations: declarations, | ||
| imports: imports, | ||
| providers: environmentProvider, |
There was a problem hiding this comment.
logic: environmentProvider could be undefined - add null check similar to other parameters
| providers: environmentProvider, | |
| providers: environmentProvider ?? [], |
|
Amazing work, @christoph-rogalla!!! @valentinpalkovic should this be labeled with a breaking change label? Or is it fully backward compatible? |
There was a problem hiding this comment.
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
| private imports: any[] = []; | ||
|
|
||
| private declarations: any[] = []; | ||
|
|
||
| private componentProviders: any[] = []; |
There was a problem hiding this comment.
style: Consider using more specific types instead of any[] for better type safety
|
@shilman, I don't think we can merge it in the next few days. The plan was to make the TestBed API work, get all of our Angular stories green, and then think about a way to integrate it in a non-breaking way in a second step, so that people have time in 9 to migrate, and in Storybook 10, we will remove the old renderer. |
# Conflicts: # code/frameworks/angular/package.json
2e6618a to
49c603c
Compare
… enhanced component rendering
|
@christoph-rogalla any chance to get this over the finish line? |
|
@valentinpalkovic Is there any chance that a Storybook core maintainer can take over this PR to finish this? It's been a while now that we can inject dependencies at the Component level in Angular and sadly we have ugly workarounds in my our codebase to inject mocked dependencies in page level components. |
|
Some of our internal story tests are still failing, and we currently have no internal resources to bring this PR across the finish line. Is someone from @storybookjs/angular interested in fixing the remaining issues? I would also be happy if someone from the community took over the remaining work. |
|
Closing for now due to its out-of-date state |
Targets #31088
What I did
The PR modifies the component creation for a story using Angular's TestBed API. The PR includes a TestBed component builder that follows the method chaining pattern. It carries all the component initialization data.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This pull request has been released as version
0.0.0-pr-31311-sha-927762ce. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-31311-sha-927762ce sandboxor in an existing project withnpx storybook@0.0.0-pr-31311-sha-927762ce upgrade.More information
0.0.0-pr-31311-sha-927762ceangular-dependencies927762ce1753300341)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=31311Greptile Summary
Introduces TestBed API integration for Angular component rendering in Storybook, with significant architectural changes to support component testing and lifecycle management.
TestBedComponentBuilderincode/frameworks/angular/src/client/angular-beta/utils/TestBedComponentBuilder.tsimplementing the builder pattern for TestBed configurationcode/frameworks/angular/src/client/angular-beta/TestBedRenderer.tscode/frameworks/angular/package.jsonwith expanded peer dependency rangecode/frameworks/angular/template/stories/demonstrating TestBed integrationcode/frameworks/angular/src/client/config.tsto prevent test environment leaks