The Wayback Machine - https://web.archive.org/web/20201023165239/https://github.com/angular/angular-cli/issues/14532
Skip to content

/ angular-cli

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

Jump to bottom

Ng update forces double quotemark #14532

Open
aescarcha opened this issue May 28, 2019 · 9 comments
Open

Ng update forces double quotemark #14532

aescarcha opened this issue May 28, 2019 · 9 comments
Labels
comp: devkit/schematics ng update DX type: feature
Milestone
Backlog

Comments

@aescarcha
Copy link

@aescarcha aescarcha commented May 28, 2019

🐞 Bug report

Command (mark with an x)

- [ ] new
- [ ] build
- [ ] serve
- [ ] test
- [ ] e2e
- [ ] generate
- [ ] add
- [x] update
- [ ] lint
- [ ] xi18n
- [ ] run
- [ ] config
- [ ] help
- [ ] version
- [ ] doc

Is this a regression?

Probably, I used ng update several times already and this never happened

Description

After running ng update to update from 7.3 to 8.0.0-rc.5 I get a big amount of tslint errors because it used double quotes (") on the modified imports. IE:

-import { MatDialogModule } from '@angular/material';
+import { MatDialogModule } from "@angular/material/dialog";

All the changed imports are from material, so I can't say if it's only an issue with material

🔬 Minimal Reproduction

run ng update --next in any angular project using material

🌍 Your Environment


Angular CLI: 7.3.9
Node: 10.13.0
OS: darwin x64
Angular: 8.0.0-rc.5
... animations, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... platform-server, router

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.13.1
@angular-devkit/build-angular     0.13.1
@angular-devkit/build-optimizer   0.13.1
@angular-devkit/build-webpack     0.13.1
@angular-devkit/core              7.3.1
@angular-devkit/schematics        7.3.9
@angular/cdk                      8.0.0-rc.2
@angular/cli                      7.3.9
@angular/http                     7.2.15
@angular/material                 8.0.0-rc.2
@ngtools/webpack                  7.3.1
@schematics/angular               7.3.9
@schematics/update                0.13.9
rxjs                              6.5.2
typescript                        3.4.5
webpack                           4.29.0
@alan-agius4
Copy link
Collaborator

@alan-agius4 alan-agius4 commented May 28, 2019

This is somewhat related to the convo I had with @devversion last week over here angular/components#16086
@ngbot ngbot bot added this to the needsTriage milestone May 28, 2019
@devversion
Copy link
Member

@devversion devversion commented May 28, 2019

Yeah. Linking to the related Material issue as well: angular/components#16035 (comment)
@clydin
Copy link
Member

@clydin clydin commented May 28, 2019

As an aside, StringLiteral nodes do have an internal property (singleQuote) that can be used to force the use of a single quote when printing.
@devversion
Copy link
Member

@devversion devversion commented May 28, 2019

Thanks @clydin for mentioning this. We actually had a conversation about this in the components channel but we never got to the point of double-checking if the printer actually respects these.

https://github.com/microsoft/TypeScript/blob/6a559e37ee0d660fcc94f086a34370e79e94b17a/src/compiler/utilities.ts#L584-L590

I will spend some time looking into this now.
@devversion
Copy link
Member

@devversion devversion commented May 28, 2019

Something we also had to look into was the additional whitespace that is added to the ts.NamedImports. This is something that also deviates from the existing import that is rewritten. Ideally we'd really just run the TslintFix task for the generated/updated files to match the project rules.
devversion added a commit to devversion/material2 that referenced this issue May 28, 2019
@devversion
fix(ng-update): do not always use double quotes for generated imports
a8b1ca4 
Currently when someone uses single quote import declarations in
their project and the `ng update` migration for Material runs, the
Material imports are rewritten into multiple imports referring to
individual secondary entry-points, but the actual existing quote
style is ignored. This means that the linter can sometimes complain
after the migration has been performed.

Ideally we'd run the tslint fix task after the migration ran in order to
fix all lint rule failures which are specific to the given project, but this
is not a best-practice yet and we need to decide on the CLI-side whether
the CLI should automatically run the fixers after generator/migration
schematics ran. See: angular/angular-cli#14532

Unfortunately we can't do the same thing for the whitespace within
the `NamedImports` declaration because the whitespace for object
literal expressions is hard-coded. See:

https://github.com/microsoft/TypeScript/blob/6a559e37ee0d660fcc94f086a34370e79e94b17a/src/compiler/emitter.ts#L3796-L3797
(`emitObjectLiteralExpression` does not allow configuring the format flags)

Fixes angular#14532
@devversion devversion mentioned this issue May 28, 2019
Merged
@clydin
Copy link
Member

@clydin clydin commented May 28, 2019 •
edited

I'm actually thinking a post-generate/add/update schematic hook may be the ultimate solution with the hook passing a list of the modified/created files. Similar to a lint-staged/husky setup for git. This would allow the project to use whatever formatter desired (e.g., tslint, eslint, prettier, etc.).
jelbourn added a commit to angular/components that referenced this issue May 28, 2019
@devversion @jelbourn
fix(ng-update): do not always use double quotes for generated imports (…
Loading status checks…
a3856c7 
…#16131)

Currently when someone uses single quote import declarations in
their project and the `ng update` migration for Material runs, the
Material imports are rewritten into multiple imports referring to
individual secondary entry-points, but the actual existing quote
style is ignored. This means that the linter can sometimes complain
after the migration has been performed.

Ideally we'd run the tslint fix task after the migration ran in order to
fix all lint rule failures which are specific to the given project, but this
is not a best-practice yet and we need to decide on the CLI-side whether
the CLI should automatically run the fixers after generator/migration
schematics ran. See: angular/angular-cli#14532

Unfortunately we can't do the same thing for the whitespace within
the `NamedImports` declaration because the whitespace for object
literal expressions is hard-coded. See:

https://github.com/microsoft/TypeScript/blob/6a559e37ee0d660fcc94f086a34370e79e94b17a/src/compiler/emitter.ts#L3796-L3797
(`emitObjectLiteralExpression` does not allow configuring the format flags)

Fixes #14532
@filipesilva filipesilva modified the milestones: needsTriage, 8.0.1 May 29, 2019
@Splaktar
Copy link
Member

@Splaktar Splaktar commented May 29, 2019

The Angular Material part of this was fixed for 8.0.0 via angular/components@a3856c7.
RudolfFrederiksen added a commit to RudolfFrederiksen/material2 that referenced this issue Jun 21, 2019
@devversion @RudolfFrederiksen
fix(ng-update): do not always use double quotes for generated imports (…
bed7ca7 
…angular#16131)

Currently when someone uses single quote import declarations in
their project and the `ng update` migration for Material runs, the
Material imports are rewritten into multiple imports referring to
individual secondary entry-points, but the actual existing quote
style is ignored. This means that the linter can sometimes complain
after the migration has been performed.

Ideally we'd run the tslint fix task after the migration ran in order to
fix all lint rule failures which are specific to the given project, but this
is not a best-practice yet and we need to decide on the CLI-side whether
the CLI should automatically run the fixers after generator/migration
schematics ran. See: angular/angular-cli#14532

Unfortunately we can't do the same thing for the whitespace within
the `NamedImports` declaration because the whitespace for object
literal expressions is hard-coded. See:

https://github.com/microsoft/TypeScript/blob/6a559e37ee0d660fcc94f086a34370e79e94b17a/src/compiler/emitter.ts#L3796-L3797
(`emitObjectLiteralExpression` does not allow configuring the format flags)

Fixes angular#14532
@alan-agius4 alan-agius4 removed this from the 8.0.x milestone Jul 2, 2019
@ngbot ngbot bot added this to the Backlog milestone Jul 2, 2019
@alan-agius4 alan-agius4 removed this from the Backlog milestone Jul 2, 2019
@ngbot ngbot bot added this to the Backlog milestone Jul 2, 2019
@alan-agius4 alan-agius4 changed the title Ng update forces double quotemarkt Dec 30, 2019
@devversion devversion mentioned this issue May 25, 2020
Closed
@devversion
Copy link
Member

@devversion devversion commented May 25, 2020

I think we should update this issue to be more generic. i.e. talk about lint failures generally caused by arbitrary schematics.

The proposal should be that lint fix runs automatically after any generate schematic. That would also simplify the --lint-fix flag for @schematics/angular generate schematics.
@Splaktar
Copy link
Member

@Splaktar Splaktar commented May 25, 2020

It would be good to triage this issue as well (priority, etc) so that it gets some attention. While not super-severe, this seems to be a frequent problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: devkit/schematics ng update DX type: feature
6 participants
@Splaktar @filipesilva @devversion @aescarcha @alan-agius4 @clydin
You can’t perform that action at this time.