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(@angular/cli): update schematics to use RouterModule when --routing flag is present #27851

Merged

Conversation

theonly1me
Copy link

@theonly1me theonly1me commented Jun 14, 2024

update schematics to use RouterModule instead of RouterTestingModule when generating a new project with the --routing flag

Fixes #27691

PR Checklist

Please check to confirm your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Running ng new --inline-style false --inline-template false --package-manager bun --routing --skip-git --standalone false --strict true --ssr false --style scss my-test-app generates a new project with the RouterTestingModule which is deprecated

Issue Number: #27691

What is the new behavior?

Running ng new --inline-style false --inline-template false --package-manager bun --routing --skip-git --standalone false --strict true --ssr false --style scss my-test-app generates a new projects with the RouterModule

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

NA

@theonly1me theonly1me marked this pull request as ready for review June 14, 2024 05:26
import { AppComponent } from './app.component';

describe('AppComponent', () => {
beforeEach(async () => {
await TestBed.configureTestingModule({<% if (routing) { %>
imports: [
RouterTestingModule
RouterModule.forRoot([])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't RouterModule be sufficient in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just RouterOutlet ?

Copy link
Author

@theonly1me theonly1me Jun 14, 2024

Choose a reason for hiding this comment

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

Wouldn't RouterModule be sufficient in this case?

Yes just using RouterModule should be sufficient since we are not registering any routes, but I found this comment when looking at related issues, which is why I updated it to use RouterModule.forRoot with an empty list of routes. Please let me know if you think it is unnecessary and can be removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@atscott, what do you suggest here?

Copy link
Contributor

Choose a reason for hiding this comment

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

RouterOutlet is the minimal thing to have in the imports here. If you aren’t trying to import anything more than is necessary, that’s the thing to do.

If you want developers to not have to learn about what parts they’re using and need to import, then RouterModule.forRoot([]) makes it work for cases where RouterLink is added to the component template (RouterModule alone would not).

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your response @atscott. @alan-agius4, @eneajaho From an ease-of-use standpoint I feel it may be better to use RouterModule.forRoot([]), but I can make the change to use RouterOutlet, if you prefer that instead.

@alan-agius4 alan-agius4 added target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Jun 17, 2024
Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. Could you please update the commit message scope to @schematics/angular?

…--routing flag is present

update schematics to use RouterModule instead of RouterTestingModule when generating a new project with the --routing flag

Fixes angular#27691
@theonly1me theonly1me force-pushed the fix/replace-router-testing-module branch from aa7be7f to 27619a2 Compare June 17, 2024 08:23
@theonly1me
Copy link
Author

Thank you for your contribution. Could you please update the commit message scope to @schematics/angular?

@alan-agius4 My bad, I've updated it now.

@theonly1me theonly1me requested a review from alan-agius4 June 17, 2024 08:25
Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

LGTM

@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jun 17, 2024
@alan-agius4 alan-agius4 merged commit e3b8b78 into angular:main Jun 17, 2024
31 checks passed
@alan-agius4
Copy link
Collaborator

The changes were merged into the following branches: main, 18.0.x

@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 Jul 18, 2024
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 target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ng new creates application with deprecated RouterTestingModule
4 participants