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

HyperLife1119 · 2021-10-19T02:52:42Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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

packages/common/http/test/context_spec.ts

petebacondarwin · 2021-10-19T11:42:09Z

@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 · 2021-10-19T15:13:36Z

@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 · 2021-10-19T17:46:24Z

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 · 2021-10-20T04:15:50Z

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 · 2021-10-20T08:44:34Z

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

petebacondarwin · 2021-10-20T09:12:43Z

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

HyperLife1119 · 2021-10-20T09:13:34Z

Wait a minute, i'm recovering...

petebacondarwin · 2021-10-20T09:14:03Z

I think this is the commit you need 8ba2a36

HyperLife1119 · 2021-10-20T09:24:31Z

It should be ok now. @petebacondarwin

petebacondarwin · 2021-10-20T09:33:48Z

packages/common/http/test/context_spec.ts

      expect(context.get(IS_ENABLED)).toBe(false);
      expect([...context.keys()]).toEqual([
        IS_ENABLED
      ]);  // value from factory function is stored in the map upon access

      context.set(IS_ENABLED, true);
+      expect(context.has(IS_ENABLED)).toBe(true);


So I actually hoped for this line to be above the line that sets the value to true. I think it is a stronger test of .has() if the stored value is falsy.

HyperLife1119 · 2021-10-20T09:46:52Z

All right. @petebacondarwin

packages/common/http/test/context_spec.ts

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

Looks good to me. Thanks.

petebacondarwin

Reviewed-for: public-api

HyperLife1119 · 2021-10-20T10:14:00Z

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

AndrewKushnir

@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

jessicajaniuk

LGTM 🍪
Thanks for your contribution!

reviewed-for: public-api

atscott · 2021-11-04T18:52:19Z

This PR was merged into the repository by commit d452b38.

angular-automatic-lock-bot · 2021-12-05T16:06:14Z

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.}

google-cla bot added the cla: yes label Oct 19, 2021

pullapprove bot requested a review from IgorMinar October 19, 2021 02:52

petebacondarwin reviewed Oct 19, 2021

View reviewed changes

packages/common/http/test/context_spec.ts Show resolved Hide resolved

ngbot bot modified the milestone: Backlog Oct 19, 2021

HyperLife1119 closed this Oct 19, 2021

HyperLife1119 force-pushed the master branch from b785cb6 to 4daf631 Compare October 19, 2021 15:06

HyperLife1119 reopened this Oct 19, 2021

HyperLife1119 force-pushed the master branch from a479ff5 to ac0288f Compare October 20, 2021 02:36

petebacondarwin added the action: rerun CI at HEAD label Oct 20, 2021

ngbot bot removed the action: rerun CI at HEAD label Oct 20, 2021

HyperLife1119 force-pushed the master branch from 42bafff to 8ba2a36 Compare October 20, 2021 08:56

pullapprove bot requested review from alxhub, AndrewKushnir and atscott October 20, 2021 08:56

HyperLife1119 closed this Oct 20, 2021

HyperLife1119 force-pushed the master branch from 8ba2a36 to 9f40d2a Compare October 20, 2021 09:03

HyperLife1119 reopened this Oct 20, 2021

petebacondarwin reviewed Oct 20, 2021

View reviewed changes

HyperLife1119 force-pushed the master branch from d8e2799 to 490b05e Compare October 20, 2021 09:45

petebacondarwin reviewed Oct 20, 2021

View reviewed changes

packages/common/http/test/context_spec.ts Outdated Show resolved Hide resolved

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.

HyperLife1119 force-pushed the master branch from 445efae to c191d30 Compare October 20, 2021 10:09

petebacondarwin 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 20, 2021

petebacondarwin approved these changes Oct 20, 2021

View reviewed changes

AndrewKushnir approved these changes Oct 25, 2021

View reviewed changes

pullapprove bot requested a review from jelbourn October 25, 2021 19:06

AndrewKushnir requested a review from jessicajaniuk November 1, 2021 18:07

jessicajaniuk approved these changes Nov 1, 2021

View reviewed changes

jessicajaniuk removed request for atscott, IgorMinar, alxhub and jelbourn November 1, 2021 20:03

atscott closed this in d452b38 Nov 4, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

HyperLife1119 commented Oct 19, 2021

petebacondarwin commented Oct 19, 2021

HyperLife1119 commented Oct 19, 2021

petebacondarwin commented Oct 19, 2021

HyperLife1119 commented Oct 20, 2021 •

edited

petebacondarwin commented Oct 20, 2021

petebacondarwin commented Oct 20, 2021

HyperLife1119 commented Oct 20, 2021

petebacondarwin commented Oct 20, 2021

HyperLife1119 commented Oct 20, 2021

petebacondarwin Oct 20, 2021

HyperLife1119 commented Oct 20, 2021

petebacondarwin left a comment

petebacondarwin left a comment

HyperLife1119 commented Oct 20, 2021

AndrewKushnir left a comment •

edited

jessicajaniuk left a comment

atscott commented Nov 4, 2021

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

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

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

Conversation

HyperLife1119 commented Oct 19, 2021

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

petebacondarwin commented Oct 19, 2021

HyperLife1119 commented Oct 19, 2021

petebacondarwin commented Oct 19, 2021

HyperLife1119 commented Oct 20, 2021 • edited

petebacondarwin commented Oct 20, 2021

petebacondarwin commented Oct 20, 2021

HyperLife1119 commented Oct 20, 2021

petebacondarwin commented Oct 20, 2021

HyperLife1119 commented Oct 20, 2021

petebacondarwin Oct 20, 2021

Choose a reason for hiding this comment

HyperLife1119 commented Oct 20, 2021

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

HyperLife1119 commented Oct 20, 2021

AndrewKushnir left a comment • edited

Choose a reason for hiding this comment

jessicajaniuk left a comment

Choose a reason for hiding this comment

atscott commented Nov 4, 2021

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

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

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

HyperLife1119 commented Oct 20, 2021 •

edited

AndrewKushnir left a comment •

edited