-
Notifications
You must be signed in to change notification settings - Fork 26.6k
feat(aio): add API list page #14899
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
feat(aio): add API list page #14899
Conversation
f95eb41
to
b1627ff
Compare
23bfe64
to
5ff2d81
Compare
@@ -28,6 +28,7 @@ export class DocumentService { | |||
|
|||
private getDocument(url: string) { | |||
this.logger.log('getting document', url); | |||
url = url.match(/[^#?]*/)[0]; // strip off fragment and query |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps we ought to move the call to computePath
here and then put this stripping logic inside compute path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM. I'm just trying to get it to work for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my current PR I have done this refactoring move, so if you rebase then you could easily add this line into computePath
now.
aio/src/app/shared/api.service.ts
Outdated
private apiBase = 'content/docs/api/'; | ||
private apiListJsonDefault = 'api-list.json'; | ||
private onDestroy: Subscription; | ||
private sectionsSubject = new ReplaySubject<ApiSection[]>(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think an AsyncSubject
is simpler here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my response in the next comment
aio/src/app/shared/api.service.ts
Outdated
.subscribe(sections => this.logger.log('ApiService got API sections') ); | ||
this.fetchSections(); | ||
} | ||
return this.sectionsSubject.asObservable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we changed fetchSections()
to return a cold observable, then in here we lazily subscribe the sectionsSubject
to this cold observable, then I think we would have what we need with fewer lines. Something like:
get sections(): Observable<ApiSection[]> {
if (!this.sectionsSubject) {
this.sectionsSubject = new AsyncSubject();
this.onDestroy = this.fetchSections().subject(this.sectionsSubject);
}
return sectionsSubject.asObservable();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you misunderstand the purpose of fetchSections
which deliberately returns void
.
It's job is to get the sections, either the first time or (if we need it to) on a refresh. It would handle retry, etc. if that became necessary. Then it pumps the result into the sections
observable by way of the sectionsSubject
. It is intrinsically "hot". It is not subcribeable.
If you rewrite as you've written (which is how I started), you cannot refresh. There's nothing to call that would pump a refreshed value into the subject.
If we don't want the refresh potential, then we don't need a subject of any kind. We just call this.fetchSections().publishReplay
(which I also did in an earlier version).
See Ben Lesh's proposed shareReplay
operator for another confirmation of my choice of ReplaySubject
: ReactiveX/rxjs#2443
FWIW, the choice of AsyncSubject
over ReplaySubject
would be irrelevant (or possibly wrong) because the http
within fetchSections
calls complete
.
TBH, this was oh-so-much-simpler in my first promise-based implementation that didn't care about refresh or observables. I almost went that way before deciding that, if we're going Observable somewhere, we might as well stick with it everywhere.
aio/src/app/shared/api.service.ts
Outdated
}); | ||
}) | ||
.do(() => this.logger.log(`Got API sections from ${url}`)) | ||
.subscribe( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By doing the subscription in the sections
getter we could avoid this subscription code here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See earlier comment about the role of fetchSections
in a "refresh" scenario. I've confirmed that it works as intended by wiring a "Refresh" button to the component.
Maybe this is all YAGNI. But if it is, I don't really know why we're bothering with an Observable
at all. This is a one-and-done operation.
I have the same quandry w/r/t navigation JSON
aio/src/app/shared/api.service.ts
Outdated
} | ||
); | ||
|
||
function normalizeItem(item: ApiItem) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's modify dgeni to produce output that is already normalized for our purposes.
It is doing loads of manipulation there so we might as well make it produce what we want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM. I hoped that would be your response. This was a shim to get me from an existing api.json
(which I still tweaked to correct the paths) to a working example.
We can fix this in a subsequent dgeni PR, yes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep!
aio/src/app/shared/api.service.ts
Outdated
|
||
private apiBase = 'content/docs/api/'; | ||
private apiListJsonDefault = 'api-list.json'; | ||
private onDestroy: Subscription; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this conventional? I would have expected to give this subscription a name that refers to the thing it is subscribed to. E.g. sectionsSubscription
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is conventional. It's an Alex R. pattern for Components that allows us to have ONE something to destroy and the name conveys its purpose.
In the Component pattern, it's actually a subject. You attach it to every observable with takeUntil(onDestroy)
. Then call onDestroy.next()
in ngOnDestroy
and they're all cancelled.
I just took a short-cut here but kept the name to convey intent rather than implementation.
aio/src/app/shared/api.service.ts
Outdated
import 'rxjs/add/operator/catch'; | ||
import 'rxjs/add/operator/do'; | ||
import 'rxjs/add/operator/map'; | ||
import 'rxjs/add/operator/publishReplay'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, catch
moved into the second callback in subscribe
and publishReplay
was from an earlier, pre-refresh implementation. The do
and map
are still needed.
if (this.status.name !== 'all') { addParm('status', this.status.name); } | ||
if (this.type.name !== 'all') { addParm('type', this.type.name); } | ||
|
||
window.history.replaceState({}, 'API Search', window.location.pathname + params); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should move something like this into the LocationService
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that. You mentioned wanting to do something along these lines for search as well. I decided to keep this PR focused and get it to work. We can refactor later when we know what we want.
const {query, status, type} = search; | ||
this.query = query || ''; | ||
// Hack: can't bind to query because input cursor always forced to end-of-line. | ||
this.queryEl.nativeElement.value = this.query; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if someone types into the search query box, it updates the browser address, which in turn updates the value of the query box.
Could we optimise it so that we don't update the query box if the URL query is the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wardbell This bit here is my biggest concern of the overall architecture of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if someone types into the search query box, it updates the browser address, which in turn updates the value of the query box.
I don't think that's true. Updating the address has no effect on the query box. Notice that <input>
's value is not bound. I had to remove the binding and substitute ViewChild('filter')
to prevent the unwanted cursor behavior (thus the hack).
I didn't love this either. But I couldn't find another, easier way. We don't have an obvious equivalent in Angular to "bind once", AFAIK
|
||
window.history.replaceState({}, 'API Search', window.location.pathname + params); | ||
|
||
function addParm(key: string, value: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addParam?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good @wardbell.
I added some minor comments about observables and where things should live.
Mostly it would be good to see some tests in here :-)
const params = path.substr(q + 1).split('&'); | ||
params.forEach(p => { | ||
const pair = p.split('='); | ||
search[pair[0]] = pair[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need decodeUriComponent
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my request to you for help on this (above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For decoding, it would be:
-search[pair[0]] = pair[1];
+search[pair[0]] = decodeURIComponent(pair[1]);
Not sure if/how this is affected in non-browser environments.
aio/src/app/shared/api.service.ts
Outdated
|
||
ngOnDestroy() { | ||
if (this.onDestroy) { | ||
this.onDestroy.unsubscribe(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what happens if this gets called while an http request is in flight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the request will be aborted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OOC, how does Http know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the beauty of Observable over Promise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But who tells XHRConnection
that it needs to abort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you unsubscribe from an observable, it calls a function to destroy the observable, with is https://github.com/angular/angular/blob/master/modules/%40angular/http/src/backends/xhr_backend.ts#L145-L148 in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but who unsubscribes from the observable returned by http.get(url)...
(subscribed here)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gkalpak You are correct that nothing abort the fetch call. The onDestroy
only unsubscribes the subscribers to the sections
Observable. The only subscriber to the http
call is the service itself.
This isn't a practical problem because this service is never torn down and so the whole thing is moot. But I was trying to be good and prepare for a change of heart in the future (YAGNI!).
If I'm going to do it, I should do it right. So, in the interest of science, I will redo it properly, using the Alex R. pattern with onDestroy
and takeUntil
.
Tests would help, eh? Those are next after the update.
aio/src/app/shared/api.service.ts
Outdated
private apiBase = 'content/docs/api/'; | ||
private apiListJsonDefault = 'api-list.json'; | ||
private onDestroy: Subscription; | ||
private sectionsSubject = new ReplaySubject<ApiSection[]>(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not BehaviorSubject
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BehaviorSubject starts with an initial value, which is not what we want. The first (and only) value in the stream should be the response. (Which is also why I think we should use AsyncSubject
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petebacondarwin Is correct about why not BehaviorSubject
. See my reason for using ReplaySubject
instead of AsyncSubject
.
aio/src/app/shared/api.service.ts
Outdated
function normalizeItem(item: ApiItem) { | ||
item.name = item.title.toLowerCase(); | ||
// convert 'secure' property to boolean `securityRisk` | ||
item.securityRisk = item.secure !== 'false'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know the semantics of secure
and securityRisk
, but mapping secure: 'false'
to securityRisk: false
seems counter-intuitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a shim. I took the api.json
as given and made the best of it. @petebacondarwin will cure in dgeni in a future PR. IMO, the JSON property should be securityRisk
and should be true
if it is a security risk.
Curiously, the data for security
in the current JSON is either "false"
or some lengthy explanatory string that we don't show anywhere. Maybe @petebacondarwin can explain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember, but that stuff was always a bit of a WIP anyway. We can tidy it all up now.
aio/src/app/shared/api.service.ts
Outdated
import 'rxjs/add/operator/catch'; | ||
import 'rxjs/add/operator/do'; | ||
import 'rxjs/add/operator/map'; | ||
import 'rxjs/add/operator/publishReplay'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary?
aio/src/app/shared/api.service.ts
Outdated
import { Observable } from 'rxjs/Observable'; | ||
import { ReplaySubject } from 'rxjs/ReplaySubject'; | ||
import { Subscription } from 'rxjs/Subscription'; | ||
import 'rxjs/add/operator/catch'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it appears not. I suspect that originally @wardbell was using it but then switched to the subscribe
below with a traditional try-catch block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Observable
is only necessary because of explicit return type onget sections()
, a habit I got into for our docs. I'll remove. -
ReplaySubject
andSubscription
are used. -
do
andmap
are used. -
catch
andpublishReplay
will be removed.
/* | ||
* API List & Filter Component | ||
* | ||
* A page displaying all of the angular API methods available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really about just methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated comment
|
||
// Get filter values from search params on the URL | ||
private getFilterValues() { | ||
const search = this.locationService.search(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the intermediate search
variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apparently not but not a blocker :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
window.history.replaceState({}, 'API Search', window.location.pathname + params); | ||
|
||
function addParm(key: string, value: string) { | ||
params += (params ? '&' : '?') + `${key}=${value}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not likely to bite us, but encodeUriComponent
would be safer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to do that but wasn't sure exactly how to apply it and how to decode it in getFilterSection
. Would you please supply the correct code?
aio/src/app/shared/api.service.ts
Outdated
get sections(): Observable<ApiSection[]> { | ||
if (this.firstTime) { | ||
this.firstTime = false; | ||
// makes sectionsSubject hot; subscribe ensures stays alive (always refCount > 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me realise that the NavigationService
is returning the http observable for the navigationViews
property. I wonder if it is triggering the request twice for the two different menus (SideNav
and TopBar
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is!! I will fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #14924
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the DocumentService
also had this problem. Fixed in #14924
c4ed9c2
to
8b2c23f
Compare
aa58453
to
a3fad3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good enough to get us moving.
It should be followed up with a series of PRs to:
- add tests
- change dgeni to produce nicer api.json output
- use PlatformLocation.search
- move the fragment stripping to the
computePath
function
aio/src/app/shared/api.service.ts
Outdated
} | ||
); | ||
|
||
function normalizeItem(item: ApiItem) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep!
aio/src/app/shared/api.service.ts
Outdated
function normalizeItem(item: ApiItem) { | ||
item.name = item.title.toLowerCase(); | ||
// convert 'secure' property to boolean `securityRisk` | ||
item.securityRisk = item.secure !== 'false'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember, but that stuff was always a bit of a WIP anyway. We can tidy it all up now.
go(url: string) { | ||
url = this.location.normalize(url); | ||
this.location.go(url); | ||
this.urlSubject.next(url); | ||
} | ||
|
||
search(): { [index: string]: string; } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just delegate to PlatformLocation.search()
here rather than coding it ourselves?
That would save the hassle of decoding the url components.
window.history.replaceState({}, 'API Search', window.location.pathname + params); | ||
|
||
function addParam(key: string, value: string) { | ||
params += (params ? '&' : '?') + `${key}=${value}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WRT encoding URI component, here is what I had in mind:
-params += (params ? '&' : '?') + `${key}=${value}`;
+params += (params ? '&' : '?') + `${key}=${window.encodeURIComponent(value)}`;
AFAICT, this is only relevant if status.name
or type.name
contain characters that have different meaning when appended to the pathname. E.g. if .name
contains &
or #
.
aio/src/app/shared/api.service.ts
Outdated
import 'rxjs/add/operator/map'; | ||
import 'rxjs/add/operator/takeUntil'; | ||
|
||
import { Logger } from 'app/shared/logger.service'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a convention about naming services: Do or don't suffix with Service
?
Currently, most of our services have the suffix (e.g. ApiService
, LocationService
, NavigationService
, etc). Logger
doesn't. Nor do the build-in services.
What does the style guide suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CLI tool is defaulting to postfixing when you use ng g service ...
.
I suspect that @wardbell will say that he doesn't like that default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The style guide encourages the Service
suffix but does NOT mandate it. I states explicitly that if the name of the class is obviously a service, you can omit it. Logger
is the example given ;-)
const params = path.substr(q + 1).split('&'); | ||
params.forEach(p => { | ||
const pair = p.split('='); | ||
search[pair[0]] = pair[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For decoding, it would be:
-search[pair[0]] = pair[1];
+search[pair[0]] = decodeURIComponent(pair[1]);
Not sure if/how this is affected in non-browser environments.
aio/src/app/shared/api.service.ts
Outdated
import 'rxjs/add/operator/map'; | ||
import 'rxjs/add/operator/takeUntil'; | ||
|
||
import { Logger } from 'app/shared/logger.service'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CLI tool is defaulting to postfixing when you use ng g service ...
.
I suspect that @wardbell will say that he doesn't like that default.
(status === 'security-risk' && item.securityRisk); | ||
}; | ||
|
||
private setLocationSearch() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A large part of this function needs to be moved into the Location service. We should not be hitting window.history
in a component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again I think that judicious use of PlatformLocation
might make this a lot cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PlatformLocation doesn't do much but the window.history
part does need to move and has been moved in the latest version of this PR.
|
||
this.apiService.sections.subscribe(sections => { | ||
this.sections = sections; | ||
this.applyFilters(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was it a conscious decision to avoid the Observable world in this component?
I imagine that Mr Lesh would be saying that updates to the filters are actually observable sequences that should be combined together with the observable sequence of the sections
, resulting in a stream of filtered sections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK ... we had hours of "fun" making this observable. The code became more obscure without being shorter but the gods of reactive purity have been placated.
*/ | ||
|
||
import { Component, ElementRef, OnInit, ViewChild } from '@angular/core'; | ||
import { LocationService } from '../../shared/location.service'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to use relative imports like this. The webpack setup has aliases such as app
. So these can be
import { LocationService } from 'app/shared/location.service';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know that. Fixed.
|
||
import { Component, ElementRef, OnInit, ViewChild } from '@angular/core'; | ||
import { LocationService } from '../../shared/location.service'; | ||
import { ApiItem, ApiSection, ApiService } from '../../shared/api.service'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the API service stored in the shared
folder?
Given that we have folders for navigation
, documents
and search
, then I would expect it either to have a folder of its own, or to appear in the navigation folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it belongs here either. I thought I was following the lead of other services here.
Originally it was in .../api/
which is the only thing that needs it.
I'll move it back there until we have a compelling reason to do otherwise.
2f9d27a
to
4e65b0a
Compare
The angular.io preview for 76cd1ca is available here. |
76cd1ca
to
08cad6a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've responded favorably to this round of comments.
*/ | ||
|
||
import { Component, ElementRef, OnInit, ViewChild } from '@angular/core'; | ||
import { LocationService } from '../../shared/location.service'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know that. Fixed.
|
||
import { Component, ElementRef, OnInit, ViewChild } from '@angular/core'; | ||
import { LocationService } from '../../shared/location.service'; | ||
import { ApiItem, ApiSection, ApiService } from '../../shared/api.service'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it belongs here either. I thought I was following the lead of other services here.
Originally it was in .../api/
which is the only thing that needs it.
I'll move it back there until we have a compelling reason to do otherwise.
aio/src/app/shared/api.service.ts
Outdated
import 'rxjs/add/operator/map'; | ||
import 'rxjs/add/operator/takeUntil'; | ||
|
||
import { Logger } from 'app/shared/logger.service'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The style guide encourages the Service
suffix but does NOT mandate it. I states explicitly that if the name of the class is obviously a service, you can omit it. Logger
is the example given ;-)
(status === 'security-risk' && item.securityRisk); | ||
}; | ||
|
||
private setLocationSearch() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PlatformLocation doesn't do much but the window.history
part does need to move and has been moved in the latest version of this PR.
|
||
this.apiService.sections.subscribe(sections => { | ||
this.sections = sections; | ||
this.applyFilters(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK ... we had hours of "fun" making this observable. The code became more obscure without being shorter but the gods of reactive purity have been placated.
|
||
// TODO: Could we just delegate to PlatformLocation.search() here rather than coding it ourselves? | ||
// That would save the hassle of decoding the url components. | ||
search(): { [index: string]: string; } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PlatformLocation.search
returns the query string. It takes an import and an injection to get it here where it saves exactly two lines. I decided against doing that.
b631b8b
to
e6f9208
Compare
The angular.io preview for b631b8b is available here. |
@@ -20,6 +20,7 @@ export class LocationService { | |||
}); | |||
} | |||
|
|||
// TODO?: ignore if url-without-hash-or-search matches current location? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the DocService strips off the query and hash, it will not emit a new doc url unnecessarily.
So I think we can safely leave them on here at this point - in case some other service does care about these things.
(i.e. you can remove this comment :-) )
LGTM - please squash and then add the merge label so the caretaker can pick it up. |
// Todo: may need to debounce as the original did | ||
// although there shouldn't be any perf consequences if we don't | ||
setQuery(query: string) { | ||
this.setSearchCriteria({query: (query || '').toLowerCase().trim() }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it was a bug that we were not searching case insensitive but I see here now that we are lowercasing what is typed. So no bug :-)
Of course you would have had a test that checked this and then I would have known that it was covered.
e6f9208
to
adf22be
Compare
The angular.io preview for e6f9208 is available here. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Add API page based on page and directive in angular.io v.1
closes #14886
Preliminary conversion ready for review. Needs:
api-list.component.scss
[POSTPONED]api.html
(currently incontent/marketing
... feels wrong).[for @petebacondarwin]embedded
folderPlease check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")