Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Angular app-shell schematic to explicitly set the component type option to an empty string (type: ''). This prevents the generated app-shell component from inheriting a custom component type from the workspace defaults, which would otherwise alter the file and class names and break the expected AppShell import. Additionally, a unit test has been added to verify this behavior by emulating workspace defaults. There are no review comments to address, and the changes look solid.
`ng generate app-shell` adds an `AppShell` import from `./app-shell/app-shell`, but the component it
generates inherits the workspace `type` option. In workspaces that set
`"@schematics/angular:component": { "type": "component" }`, such as those updated to v20, the
component was generated as `app-shell.component.ts` and the import failed to resolve.
Pass an empty `type`, as the library schematic does (661609e).
Refs angular#34449
7ece3b8 to
c7f3fce
Compare
alan-agius4
left a comment
alan-agius4
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Left a comment regarding the test in packages/schematics/angular/app-shell/index_spec.ts.
| it('should not inherit the component `type` from the workspace defaults', async () => { | ||
| // The CLI applies the `schematics` defaults of `angular.json` to nested schematics as well. | ||
| // Emulate it with a fresh runner so that other tests are not affected. | ||
| const runner = new SchematicTestRunner( | ||
| '@schematics/angular', | ||
| require.resolve('../collection.json'), | ||
| ); | ||
| runner['_engineHost'].registerOptionsTransform( | ||
| (schematic: { name: string }, options: Record<string, unknown>) => | ||
| schematic.name === 'component' && options['type'] === undefined | ||
| ? { ...options, type: 'component' } | ||
| : options, | ||
| ); | ||
|
|
||
| const tree = await runner.runSchematic('app-shell', defaultOptions, appTree); | ||
|
|
||
| expect(tree.exists('/projects/bar/src/app/app-shell/app-shell.ts')).toBe(true); | ||
| expect(tree.exists('/projects/bar/src/app/app-shell/app-shell.component.ts')).toBe(false); | ||
| const content = tree.readContent('/projects/bar/src/app/app.config.server.ts'); | ||
| expect(content).toContain(`import { AppShell } from './app-shell/app-shell';`); | ||
| }); |
There was a problem hiding this comment.
This runner['_engineHost'].registerOptionsTransform(...) setup seems redundant and accesses a private property on SchematicTestRunner. SchematicTestRunner does not read angular.json schematic defaults (which are applied by the CLI's SchematicsCommandModule), so registering a custom transform here only tests the inline callback against the literal type: '' option rather than actual workspace configuration.
Consistent with packages/schematics/angular/library/index.ts (which also passes type: '' to the component schematic), the existing unit tests above already verify that /projects/bar/src/app/app-shell/app-shell.ts and import { AppShell } from './app-shell/app-shell'; are generated, or if we want to test workspace schematic defaults end-to-end, that should be done in an E2E test or by supporting workspace defaults properly rather than monkey-patching _engineHost.
alan-agius4
added
action: cleanup
2 participants
ng generate app-shelladds anAppShellimport from./app-shell/app-shell, but the component it generates inherits the workspacetypeoption. In workspaces that set"@schematics/angular:component": { "type": "component" }, such as those updated to v20, the component was generated asapp-shell.component.tsand the import failed to resolve.Pass an empty
type, as the library schematic does (661609e).Fixes #34449