Skip to content

feat(@schematics/angular): remove useless import for Ivy#11874

Merged
vikerman merged 1 commit intoangular:masterfrom
cexbrayat:fix/ivy-imports
Aug 28, 2018
Merged

feat(@schematics/angular): remove useless import for Ivy#11874
vikerman merged 1 commit intoangular:masterfrom
cexbrayat:fix/ivy-imports

Conversation

@cexbrayat
Copy link
Copy Markdown
Member

Using cli 6.2.0-beta.2 with the new experimentalIvy flag leads to:

ERROR in src/app/app.module.ts(1,1): error TS6133: 'BrowserModule' is declared but its value is never read.

@@ -1,4 +1,4 @@
import { BrowserModule } from '@angular/platform-browser';
<% if (!experimentalIvy) { %>import { BrowserModule } from '@angular/platform-browser';<% } %>
Copy link
Copy Markdown
Collaborator

@alan-agius4 alan-agius4 Aug 13, 2018

Choose a reason for hiding this comment

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

I think there is the same problem for AppRoutingModule, as at the moment it looks like this is only being used if it's not experimentalIvy

imports: [<% if (!experimentalIvy) { %>
BrowserModule<% if (routing) { %>,
AppRoutingModule<% } %>

I think that AppRoutingModule shouldn't be effect by the fact if experimentalIvy is enabled or disabled.

That said, maybe @hansl can confirm about this, as he initial working on implementing the experimentalIvy support

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You're right. I generated it without the routing flag so I didn't see it, but I can add it if @hansl confirms.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes. I don't even know if we support router in Ivy.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I amended the commit to remove the AppRoutingModule if the experimentalIvy flag is used.

Using cli `6.2.0-beta.2` with the new `experimentalIvy` flag leads to:

    ERROR in src/app/app.module.ts(1,1): error TS6133: 'BrowserModule' is declared but its value is never read.
@alexeagle
Copy link
Copy Markdown
Contributor

We should not promote unused variable to a compiler error. Do we have --noUnusedLocals in the tsconfig? Could we remove that please?

@cexbrayat
Copy link
Copy Markdown
Member Author

@alexeagle no worries, it's not in the default tsconfig. Sorry if my description was misleading: I just wanted to point out that the useless imports should be removed.

@vikerman
Copy link
Copy Markdown
Contributor

Rerunning failed tests...

@vikerman vikerman added target: major This PR is targeted for the next major release and removed target: major This PR is targeted for the next major release labels Aug 28, 2018
@vikerman
Copy link
Copy Markdown
Contributor

Please assign appropriate merge target

@vikerman vikerman added the target: major This PR is targeted for the next major release label Aug 28, 2018
@vikerman vikerman merged commit 3f94b2d into angular:master Aug 28, 2018
@angular-automatic-lock-bot
Copy link
Copy Markdown

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 Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants