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

Improve async handling in generated code #18783

Closed
wants to merge 2 commits into from

Conversation

apeeters
Copy link

This PR fixes two tslint errors in generated projects:

  • Promise returning method should be async
  • Avoid floating promises

@alan-agius4
Copy link
Collaborator

alan-agius4 commented Sep 14, 2020

The “floating” promises are intended. See: #17652 (comment)

@apeeters
Copy link
Author

The “floating” promises are intended. See: #17652 (comment)

Unfortunately the PR mentioned in that comment (#15472) is closed. I can't find any documentation or plan on how this will be fixed/resolved.

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.

We had a chat around this yesterday during our team meeting and doing this change shouldn't break Protractor. Therefore this should be good to go after

  • Update the commit message scopes to @schematics/angular
  • Dropping Jasmine bump commit since this seems redundant.

@alan-agius4 alan-agius4 added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 15, 2020
expect(page.getTitleText()).toEqual('integration-project app is running!');
it('should display welcome message', async () => {
await page.navigateTo();
await expect(await page.getTitleText()).toEqual('integration-project app is running!');
Copy link
Member

Choose a reason for hiding this comment

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

The outer await should not be needed here and in the other tests.

Copy link
Author

Choose a reason for hiding this comment

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

The tsconfig.json.template file in packages/schematics/angular/e2e/files specifies jasminewd2 in types. The method signatures in these type definitions return Promises. So I believe this change is needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It returns a promise because it's a generic, therefore if you await the return type is no longer a promise.

declare function expect<T>(actual: ArrayLike<T>): jasmine.ArrayLikeMatchers<T>;

@apeeters
Copy link
Author

  • Update the commit message scopes to @schematics/angular
  • Dropping Jasmine bump commit since this seems redundant.

Fixed these remarks and rebased the PR.

@clydin
Copy link
Member

clydin commented Oct 6, 2020

Thank you for the contribution. This change has, however, been superseded by #18951. This will be available in the upcoming version 11.

@clydin clydin closed this Oct 6, 2020
@clydin clydin removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 6, 2020
@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 Nov 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants