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

fix(common): add HttpParamsOptions to the public api #35829

Conversation

rwlogel
Copy link

@rwlogel rwlogel commented Mar 3, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Documentation content changes
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

The HttpParamsOptions is not documented or included in the public API even though it is a constructor argument of HttpParams which is in the public API.

Issue Number: #20276

What is the new behavior?

HttpParamsOptions will be included in the public API and documented.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@rwlogel rwlogel force-pushed the docs-common-add-http-params-options-to-public-api branch from 3a6ae72 to 22be818 Compare March 3, 2020 16:07
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Mar 3, 2020
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

This is not a doc's change. Can you please change this comment to be a fix thank you

@rwlogel rwlogel force-pushed the docs-common-add-http-params-options-to-public-api branch from 22be818 to c387233 Compare March 3, 2020 16:24
@rwlogel rwlogel requested a review from IgorMinar March 3, 2020 16:25
@rwlogel rwlogel changed the title docs(common): add HttpParamsOptions to the public api fix(common): add HttpParamsOptions to the public api Mar 3, 2020
@gkalpak
Copy link
Member

gkalpak commented Mar 3, 2020

For context: #20332 and #23015

@rwlogel rwlogel force-pushed the docs-common-add-http-params-options-to-public-api branch from c387233 to faeebb9 Compare March 3, 2020 16:49
@rwlogel
Copy link
Author

rwlogel commented Mar 3, 2020

The reason I think we should expose the interface instead of making it inline as part of the HttpParams structure is because it makes extending HttpParams less complicated. This is the situation where I wanted to use it to work around #11058:

export class PlusSafeQueryEncoder implements HttpParameterCodec {
  encodeKey(key: string): string {
    return encodeURIComponent(key);
  }

  encodeValue(value: string): string {
    return encodeURIComponent(value);
  }

  decodeKey(key: string): string {
    return decodeURIComponent(key);
  }

  decodeValue(value: string): string {
    return decodeURIComponent(value);
  }
}

export class PlusSafeHttpParams extends HttpParams {
  constructor(options?: HttpParamsOptions = {} as HttpParamsOptions) {
    super({ ...options, encoder: new PlusSafeQueryEncoder() })
  }
}

@atscott atscott added the area: common Issues related to APIs in the @angular/common package label Mar 3, 2020
@ngbot ngbot bot added this to the needsTriage milestone Mar 3, 2020
@gkalpak
Copy link
Member

gkalpak commented Mar 4, 2020

Sounds reasonable to me.

You can always grab the type from the HttpParams constructor, but it still sounds reasonable to use an interface:

type MyHttpParamsOptions = ConstructorParameters<typeof HttpParams>[0];

@rwlogel rwlogel force-pushed the docs-common-add-http-params-options-to-public-api branch from faeebb9 to 34628cc Compare March 16, 2020 15:38
@pullapprove pullapprove bot requested review from alxhub and jelbourn June 8, 2020 15:50
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: public-api

@petebacondarwin petebacondarwin added area: common/http action: review The PR is still awaiting reviews from at least one requested reviewer and removed area: common Issues related to APIs in the @angular/common package comp: http labels Jul 14, 2020
@AndrewKushnir AndrewKushnir removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 28, 2020
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer and removed aio: preview labels Jul 28, 2020
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Thanks for updating this PR!

I've also checked that the HttpParamsOptions is displayed correctly (see here) and also started tests in Google's codebase (internal-only link).

Note: this PR should be ready for merge once all tests are completed and there is a final approval from the fw-http group (cc @alxhub).

Thank you.

@AndrewKushnir AndrewKushnir removed the request for review from pkozlowski-opensource July 28, 2020 22:03
@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Nov 4, 2020
Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

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

Reviewed-For: public-api

@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 10, 2020
@AndrewKushnir
Copy link
Contributor

Started a new presubmit, will keep this thread updated.

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Nov 11, 2020
@AndrewKushnir
Copy link
Contributor

FYI, presubmit is successful for the changes in this PR. Thank you.

@atscott atscott added target: minor This PR is targeted for the next minor release and removed target: major This PR is targeted for the next major release labels Nov 12, 2020
The `HttpParamsOptions` was not documented or included in the public API even
though it is a constructor argument of `HttpParams` which is a part of the
public API. This commit adds the `HttpParamsOptions` into the exports, thus
making it a part of the public API.

Resolves angular#20276
@atscott atscott force-pushed the docs-common-add-http-params-options-to-public-api branch from 51aad23 to d5e16ef Compare November 12, 2020 17:07
@atscott atscott closed this in b33b89d Nov 18, 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 Dec 19, 2020
@pullapprove pullapprove bot added the area: common Issues related to APIs in the @angular/common package label Dec 19, 2020
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: common/http area: common Issues related to APIs in the @angular/common package cla: yes target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet