X Tutup
The Wayback Machine - https://web.archive.org/web/20220130093348/https://github.com/angular/angular/pull/43887
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

Jump to bottom

feat(http): add has() method to HttpContext class #43887

Closed
wants to merge 1 commit into from
Closed

feat(http): add has() method to HttpContext class #43887

wants to merge 1 commit into from
+16 −0

Conversation

@HyperLife1119
Copy link
Contributor

@HyperLife1119 HyperLife1119 commented Oct 19, 2021

PR Checklist

Please check if 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
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Unable to determine whether a certain token exists in HttpContext.

What is the new behavior?

Use the has() method provided by HttpContext to detect whether a certain token exists

Does this PR introduce a breaking change?

  • Yes
  • No

Other information
@google-cla google-cla bot added the cla: yes label Oct 19, 2021
@pullapprove pullapprove bot requested a review from IgorMinar Oct 19, 2021
@ngbot ngbot bot added this to the Backlog milestone Oct 19, 2021
@ngbot ngbot bot added this to the Backlog milestone Oct 19, 2021
@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Oct 19, 2021

@HyperLife1119 - thanks for this PR.
Please can you rebase on the current angular/angular master to get the CI to run.
Also, please just squash all three commits together into a single one.
Finally, we might need to update the size tracking files if this new feature pushes the size of applications that use HttpContext over the current limit.

@HyperLife1119 HyperLife1119 reopened this Oct 19, 2021
@HyperLife1119
Copy link
Contributor Author

@HyperLife1119 HyperLife1119 commented Oct 19, 2021

@HyperLife1119 - thanks for this PR. Please can you rebase on the current angular/angular master to get the CI to run. Also, please just squash all three commits together into a single one. Finally, we might need to update the size tracking files if this new feature pushes the size of applications that use HttpContext over the current limit.

I think it should be ok. 😄

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Oct 19, 2021

Great! As expected we now have a couple more steps due to the new public API.

Please run yarn bazel run //packages/common:common_api.accept.

And then save it as a fixup commit (e.g. git commit -a --fixup=HEAD).

@HyperLife1119
Copy link
Contributor Author

@HyperLife1119 HyperLife1119 commented Oct 20, 2021 •
edited

Great! As expected we now have a couple more steps due to the new public API.

Please run yarn bazel run //packages/common:common_api.accept.

And then save it as a fixup commit (e.g. git commit -a --fixup=HEAD).

I finished.

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Oct 20, 2021

One more rebase please @HyperLife1119 - then I think we are good on this.

@pullapprove pullapprove bot requested review from alxhub, AndrewKushnir and atscott Oct 20, 2021
@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Oct 20, 2021

You seem to have automatically closed the PR by pushing zero commits? Did you accidentally reset the branch to master rather than rebasing?

@HyperLife1119
Copy link
Contributor Author

@HyperLife1119 HyperLife1119 commented Oct 20, 2021

Wait a minute, i'm recovering...

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Oct 20, 2021

I think this is the commit you need 8ba2a36

@HyperLife1119 HyperLife1119 reopened this Oct 20, 2021
@HyperLife1119
Copy link
Contributor Author

@HyperLife1119 HyperLife1119 commented Oct 20, 2021

It should be ok now. @petebacondarwin

@HyperLife1119
Copy link
Contributor Author

@HyperLife1119 HyperLife1119 commented Oct 20, 2021

All right. @petebacondarwin

@HyperLife1119
feat(http): add has() method to HttpContext class
c191d30 
At present, the get() method provided by HttpContext will never return null. Sometimes we need to check whether an http token is included, so add the has() method to HttpContext.
petebacondarwin
petebacondarwin approved these changes Oct 20, 2021
View changes
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Looks good to me. Thanks.

petebacondarwin
petebacondarwin approved these changes Oct 20, 2021
View changes
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Reviewed-for: public-api

@HyperLife1119
Copy link
Contributor Author

@HyperLife1119 HyperLife1119 commented Oct 20, 2021

This is my first PR to angular, thank you for your guidance. @petebacondarwin

AndrewKushnir
AndrewKushnir approved these changes Oct 25, 2021
View changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment •
edited

@HyperLife1119 thanks for contributing to Angular! 👍

I just wanted to mention that this change adds some non-tree-shakable code to app bundles in case HTTP package is used. However the added code would be small and the new API looks reasonable (and somewhat missing in the current implementation), so we can probably justify that minimal size increase.

Reviewed-for: public-api

@pullapprove pullapprove bot requested a review from jelbourn Oct 25, 2021
@AndrewKushnir AndrewKushnir requested a review from jessicajaniuk Nov 1, 2021
jessicajaniuk
jessicajaniuk approved these changes Nov 1, 2021
View changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

LGTM 🍪
Thanks for your contribution!

reviewed-for: public-api

@atscott
Copy link
Contributor

@atscott atscott commented Nov 4, 2021

This PR was merged into the repository by commit d452b38.

@atscott atscott closed this in d452b38 Nov 4, 2021
@sync-by-unito sync-by-unito bot mentioned this pull request Dec 1, 2021
Closed
@sync-by-unito sync-by-unito bot mentioned this pull request Dec 1, 2021
Closed
@sync-by-unito sync-by-unito bot mentioned this pull request Dec 1, 2021
Closed
@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Dec 5, 2021

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 Dec 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@petebacondarwin petebacondarwin

@AndrewKushnir AndrewKushnir

@jessicajaniuk jessicajaniuk

Assignees
No one assigned
Labels
action: merge cla: yes comp: common/http target: minor
Projects
None yet
Milestone
Backlog
Linked issues

Successfully merging this pull request may close these issues.

None yet
5 participants
@HyperLife1119 @petebacondarwin @atscott @AndrewKushnir @jessicajaniuk
X Tutup