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

fix(material/schematics): support standalone components in ng-add #24931

Merged
merged 1 commit into from May 19, 2022

Conversation

crisbeto
Copy link
Member

Adds code to handle apps that are bootstrapped through bootstrapApplication instead of bootstrapModule.

Adds code to handle apps that are bootstrapped through `bootstrapApplication` instead of `bootstrapModule`.
@crisbeto crisbeto added P2 The issue is important to a large percentage of users, with a workaround merge safe target: rc This PR is targeted for the next release-candidate labels May 17, 2022
@crisbeto crisbeto marked this pull request as ready for review May 17, 2022 18:42
@crisbeto crisbeto requested review from a team, andrewseguin and devversion as code owners May 17, 2022 18:42
if (importsProvidersFrom(host, mainFile, noopAnimationsModuleName)) {
context.logger.error(
`Could not set up "${browserAnimationsModuleName}" ` +
`because "${noopAnimationsModuleName}" is already imported.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we include information on where the NoopAnimationsModule is imported?

Suggested change
`because "${noopAnimationsModuleName}" is already imported.`,
`because "${noopAnimationsModuleName}" is already imported into ` +
`the "${...}" NgModule.`,

try {
addAnimationsModuleToNonStandaloneApp(host, project, context, options);
} catch (e) {
if (e instanceof SchematicsException && e.message.includes('Bootstrap call not found')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@crisbeto quick question: is there a situation now when the Bootstrap call not found is thrown? If that's the case, may be we can try to improve the messaging there as well (just change the content)?

Copy link
Member Author

Choose a reason for hiding this comment

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

It comes from the vendored utilities from the CLI repo. I'd rather avoid changing the message here since it'll be reset whenever they're updated. The only case where this will be thrown is if the main file doesn't have a bootstrapModule or bootstrapApplication call at all.

@@ -227,6 +227,52 @@ describe('ng-add schematic', () => {
expect(errorOutput.length).toBe(1);
expect(errorOutput[0]).toMatch(/Could not set up "BrowserAnimationsModule"/);
});

it('should add the BrowserAnimationsModule to a bootstrapApplication call', async () => {
appTree.delete('/projects/material/src/app/app.module.ts');

Choose a reason for hiding this comment

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

question: why do we need to delete this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't, but I figured that if we had a --standalone option for ng new, this file wouldn't exist so I wanted the test to be as close as possible to the "real thing".

try {
addAnimationsModuleToNonStandaloneApp(host, project, context, options);
} catch (e) {
if (e instanceof SchematicsException && e.message.includes('Bootstrap call not found')) {

Choose a reason for hiding this comment

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

I'm worried that "sniffing" on the exception + its error message is rather fragile (will break any time someone decides to alter the error message). Could we have a specific exception type for this case? Or some other reliable way detecting what is going on?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not a fan of it either, but I was trying to avoid changing any of the vendored schematic utilities. We have unit tests for this so if the message changes and the check doesn't hold anymore, we'll know about it.

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 guess this comes down to the common utils at some point, but for now seems fine.

@@ -9,6 +9,9 @@ integration/ng-update-v13/node_modules
integration/ng-add/.angular
integration/ng-add/node_modules

integration/ng-add-standalone/.angular
integration/ng-add-standalone/node_modules

Copy link
Member

Choose a reason for hiding this comment

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

Thx for adding it here!

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label May 19, 2022
@crisbeto crisbeto merged commit fc35e2b into angular:main May 19, 2022
crisbeto added a commit that referenced this pull request May 19, 2022
…4931)

Adds code to handle apps that are bootstrapped through `bootstrapApplication` instead of `bootstrapModule`.

(cherry picked from commit fc35e2b)
clydin added a commit to clydin/angular-cli that referenced this pull request May 24, 2022
… private export for Angular components

The standalone component helper utilities introduced into the `@angular/cdk` via
angular/components#24931 have been added to an export path (`private/components`)
within `@schematics/angular`. The export path is primarily intended for the use of the components schematics
within `@angular/cdk` and `@angular/material`. The API exported from this path is not considered public API,
does not provide SemVer guarantees, and may be modified or removed without a deprecation cycle.
clydin added a commit to clydin/angular-cli that referenced this pull request May 24, 2022
… private export for Angular components

The standalone component helper utilities introduced into the `@angular/cdk` via
angular/components#24931 have been added to an export path (`private/components`)
within `@schematics/angular`. The export path is primarily intended for the use of the components schematics
within `@angular/cdk` and `@angular/material`. The API exported from this path is not considered public API,
does not provide SemVer guarantees, and may be modified or removed without a deprecation cycle.
dgp1130 pushed a commit to angular/angular-cli that referenced this pull request May 26, 2022
… private export for Angular components

The standalone component helper utilities introduced into the `@angular/cdk` via
angular/components#24931 have been added to an export path (`private/components`)
within `@schematics/angular`. The export path is primarily intended for the use of the components schematics
within `@angular/cdk` and `@angular/material`. The API exported from this path is not considered public API,
does not provide SemVer guarantees, and may be modified or removed without a deprecation cycle.
dgp1130 pushed a commit to angular/angular-cli that referenced this pull request May 26, 2022
… private export for Angular components

The standalone component helper utilities introduced into the `@angular/cdk` via
angular/components#24931 have been added to an export path (`private/components`)
within `@schematics/angular`. The export path is primarily intended for the use of the components schematics
within `@angular/cdk` and `@angular/material`. The API exported from this path is not considered public API,
does not provide SemVer guarantees, and may be modified or removed without a deprecation cycle.

(cherry picked from commit e895fc7)
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker P2 The issue is important to a large percentage of users, with a workaround target: rc This PR is targeted for the next release-candidate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants