Skip to content

fix(@angular/cli): Do not use floating promises, we can resolve these… #17652

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.

Sign up for GitHub

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

Closed
timfrijtersppw wants to merge 2 commits into from
Closed

fix(@angular/cli): Do not use floating promises, we can resolve these… #17652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6bfe728
fix(@angular/cli): Do not use floating promises, we can resolve these…
timfrijtersppw May 6, 2020
b6f5bd4
fix(@angular/cli): Functions that return promises must be async
timfrijtersppw May 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/schematics/angular/application/other-files/app.component.spec.ts.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,16 @@ import { RouterTestingModule } from '@angular/router/testing';<% } %>
import { AppComponent } from './app.component';

describe('AppComponent', () => {
beforeEach(async(() => {
TestBed.configureTestingModule({<% if (routing) { %>
beforeEach(async () => {
return TestBed.configureTestingModule({<% if (routing) { %>
Copy link
Copy Markdown
Member

@gkalpak gkalpak May 6, 2020

Choose a reason for hiding this comment

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

Is there any point in using async if you are not awaiting the promise?
I would expect either async () => { await TestBed... } or () => { return TestBed... }.

Copy link
Copy Markdown
Member

@clydin clydin May 6, 2020

Choose a reason for hiding this comment

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

i'd probably go with await to make it more explicit that it is an asynchronous operation and also slightly simplify future user additions (either before or after the call).

Copy link
Copy Markdown
Contributor Author

@timfrijtersppw timfrijtersppw May 7, 2020

Choose a reason for hiding this comment

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

I think async shouldn't only be used when awaiting, returning the promise also showcases asynchronous behavior.

imports: [
RouterTestingModule
],<% } %>
declarations: [
AppComponent
],
}).compileComponents();
}));
});

it('should create the app', () => {
const fixture = TestBed.createComponent(AppComponent);
Expand Down
6 changes: 3 additions & 3 deletions ...t/files/__name@dasherize@if-flat__/__name@dasherize__.__type@dasherize__.spec.ts.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@ describe('<%= classify(name) %><%= classify(type) %>', () => {
let component: <%= classify(name) %><%= classify(type) %>;
let fixture: ComponentFixture<<%= classify(name) %><%= classify(type) %>>;

beforeEach(async(() => {
TestBed.configureTestingModule({
beforeEach(async () => {
return TestBed.configureTestingModule({
declarations: [ <%= classify(name) %><%= classify(type) %> ]
})
.compileComponents();
}));
});

beforeEach(() => {
fixture = TestBed.createComponent(<%= classify(name) %><%= classify(type) %>);
Expand Down
4 changes: 2 additions & 2 deletions packages/schematics/angular/e2e/files/src/app.po.ts.template
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { browser, by, element } from 'protractor';

export class AppPage {
navigateTo(): Promise<unknown> {
async navigateTo(): Promise<unknown> {
Copy link
Copy Markdown
Member

@gkalpak gkalpak May 6, 2020

Choose a reason for hiding this comment

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

FWIW, you can also use PromiseLike<...> here and not have to cast below.
Do you know if there are any downsides to using PromiseLike?

Copy link
Copy Markdown
Member

@clydin clydin May 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

PromiseLike is mainly useful when dealing with alternative implementations of promises (bluebird, etc.). The only real downside is that it won't be accepted for any consumer that expects a Promise (e.g., function myFunc(p: Promise<unknown>) {}).

Copy link
Copy Markdown
Collaborator

@alan-agius4 alan-agius4 May 7, 2020

Choose a reason for hiding this comment

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

Also, no reason for async here since you are not using await.

return browser.get(browser.baseUrl) as Promise<unknown>;
}

getTitleText(): Promise<string> {
async getTitleText(): Promise<string> {
return element(by.css('<%= rootSelector %> .content span')).getText() as Promise<string>;
}
}