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

docs(common): add HttpParamsOptions to the public API #20332

Closed

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Nov 10, 2017

PR Checklist

PR Type

[x] Documentation content changes

What is the current behavior?

HttpParamsOptions is not documented as part of the public API.
Issue Number: #20276

What is the new behavior?

HttpParamsOptions is documented as part of the public API.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@gkalpak gkalpak requested a review from alxhub November 10, 2017 11:56
@gkalpak gkalpak added area: common/http action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release type: docs labels Nov 10, 2017
@mary-poppins
Copy link

You can preview 7b3fb9b at https://pr20332-7b3fb9b.ngbuilds.io/.

@trotyl
Copy link
Contributor

trotyl commented Nov 11, 2017

Shouldn't it be added to public API guard as well (by re-export in entry)? Currently it still cannot be imported via .ts file.

PS: it's also not in doc as well.

@gkalpak gkalpak force-pushed the docs-common-export-HttpParamsOptions branch from 7b3fb9b to 94e1241 Compare November 11, 2017 09:42
@mary-poppins
Copy link

You can preview 94e1241 at https://pr20332-94e1241.ngbuilds.io/.

@gkalpak gkalpak force-pushed the docs-common-export-HttpParamsOptions branch from 94e1241 to 4e8f151 Compare November 13, 2017 10:14
@mary-poppins
Copy link

You can preview 4e8f151 at https://pr20332-4e8f151.ngbuilds.io/.

@gkalpak gkalpak force-pushed the docs-common-export-HttpParamsOptions branch from 4e8f151 to 15a36f7 Compare November 13, 2017 11:15
@mary-poppins
Copy link

You can preview 15a36f7 at https://pr20332-15a36f7.ngbuilds.io/.

@sunew
Copy link

sunew commented Nov 28, 2017

Any news on this one?

@gkalpak
Copy link
Member Author

gkalpak commented Jan 4, 2018

/ping @alxhub

@alxhub
Copy link
Member

alxhub commented Jan 24, 2018

Hi @gkalpak,

I actually think the right approach here is to get rid of the interface type and inline its definition into the HttpParams constructor.

@gkalpak gkalpak force-pushed the docs-common-export-HttpParamsOptions branch from 15a36f7 to f66b3c9 Compare January 25, 2018 10:47
@mary-poppins
Copy link

You can preview f66b3c9 at https://pr20332-f66b3c9.ngbuilds.io/.

@gkalpak gkalpak force-pushed the docs-common-export-HttpParamsOptions branch from f66b3c9 to f1cb452 Compare January 25, 2018 11:51
@mary-poppins
Copy link

You can preview f1cb452 at https://pr20332-f1cb452.ngbuilds.io/.

@gkalpak gkalpak force-pushed the docs-common-export-HttpParamsOptions branch from f1cb452 to 11cafd3 Compare January 25, 2018 12:48
@gkalpak
Copy link
Member Author

gkalpak commented Jan 25, 2018

@alxhub, PTAL

@mary-poppins
Copy link

You can preview 11cafd3 at https://pr20332-11cafd3.ngbuilds.io/.

@alxhub alxhub requested a review from mhevery January 26, 2018 16:16
* String representation of the HTTP params in URL-query-string format. Mutually exclusive with
* `fromObject`.
*/
fromString?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding doc comments does not seem to work here: https://pr20332-11cafd3.ngbuilds.io/api/common/http/HttpParams

Please file an issue with AIO

mhevery added a commit that referenced this pull request Feb 8, 2018
@mhevery
Copy link
Contributor

mhevery commented Feb 8, 2018

@gkalpak
Copy link
Member Author

gkalpak commented Feb 9, 2018

Any idea on how to fix it?

@mhevery
Copy link
Contributor

mhevery commented Feb 9, 2018

Here is the error:

error(s):
packages/common/http/src/params.closure.js:131: 
Originally at:
packages/common/http/src/params.ts:105: ERROR - Property disambiguator skipping all instances of property fromObject because of type {
  encoder: (module$contents$packages$common$http$src$params_HttpParameterCodec|undefined),
  fromObject: (Object<string,(Array<string>|string)>|undefined),
  fromString: (string|undefined)
} node GETPROP 131 [length: 18] [originalname: fromObject] [source_file: packages/common/http/src/params.closure.js] : (Object<string,(Array<string>|string)>|undefined). 

@gkalpak
Copy link
Member Author

gkalpak commented Feb 10, 2018

It seems that this error comes from here, but I have no idea how to solve it 😁
Maybe adding the interface back (and adding it to the public API - which @alxhub would prefer to avoid) would help closure disambiguate the fromObject property.

jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018
jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018
@IgorMinar
Copy link
Contributor

@matsko matsko requested review from petebacondarwin and removed request for petebacondarwin March 22, 2018 18:45
@angular angular deleted a comment from petebacondarwin Mar 22, 2018
matsko pushed a commit that referenced this pull request Mar 23, 2018
@matsko matsko closed this in 7b7757d Mar 23, 2018
@gkalpak gkalpak deleted the docs-common-export-HttpParamsOptions branch March 23, 2018 20:42
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
matsko added a commit to matsko/angular that referenced this pull request Mar 26, 2018
@matsko
Copy link
Contributor

matsko commented Mar 26, 2018

@gkalpak @alxhub @mhevery this got reveted because of G3 failures.

matsko added a commit that referenced this pull request Mar 26, 2018
gkalpak added a commit to gkalpak/angular that referenced this pull request Mar 27, 2018
@gkalpak
Copy link
Member Author

gkalpak commented Mar 27, 2018

Resubmitted as #23015.

gkalpak added a commit to gkalpak/angular that referenced this pull request Jan 14, 2019
gkalpak added a commit to gkalpak/angular that referenced this pull request Jan 16, 2019
gkalpak added a commit to gkalpak/angular that referenced this pull request Apr 3, 2019
gkalpak added a commit to gkalpak/angular that referenced this pull request Jul 25, 2019
@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 Sep 13, 2019
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 cla: yes state: blocked target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet