Repository navigation
Conversation
IgorMinar
changed the title
alexeagle
added
the
area: packaging
aed6612 to
c0bf0ad
Compare
There was a problem hiding this comment.
For some reason this doesn't seem to work. The symptom I see is that the observable that subscribes to the result of getConfig (https://github.com/angular/angular/pull/22573/files#diff-447af6fbd9dbfe9744627f202c8440c0R33) doesn't get the 'Something bad happened;...' string via the error channel.
There was a problem hiding this comment.
Where is it being called? My guess is you're handling the error somewhere else, because that function is pretty simple... basically new Observable(s => s.error(yourError))
There was a problem hiding this comment.
resolved via a fix of a bad import in rxjs
|
You can preview f22c86d at https://pr22573-f22c86d.ngbuilds.io/. |
|
You can preview 1aea83c at https://pr22573-1aea83c.ngbuilds.io/. |
|
Hi @IgorMinar! This PR has merge conflicts due to recent upstream merges. |
|
You can preview e0bc680 at https://pr22573-e0bc680.ngbuilds.io/. |
|
You can preview ad72391 at https://pr22573-ad72391.ngbuilds.io/. |
|
You can preview f3f787c at https://pr22573-f3f787c.ngbuilds.io/. |
|
You can preview b6c467c at https://pr22573-b6c467c.ngbuilds.io/. |
|
Hi @IgorMinar! This PR has merge conflicts due to recent upstream merges. |
2 similar comments
|
Hi @IgorMinar! This PR has merge conflicts due to recent upstream merges. |
|
Hi @IgorMinar! This PR has merge conflicts due to recent upstream merges. |
|
You can preview ec6ff11 at https://pr22573-ec6ff11.ngbuilds.io/. |
jasonaden
left a comment
jasonaden
left a comment
There was a problem hiding this comment.
Ignore comments for simplification changes. Will tack on to an add-on PR.
I removed a couple TODOs & pushed a fixup commit. Looks good!
| expanded$, (rootSegmentGroup: UrlSegmentGroup) => this.createUrlTree( | ||
| rootSegmentGroup, this.urlTree.queryParams, this.urlTree.fragment !)); | ||
| return _catch.call(urlTrees$, (e: any) => { | ||
| const urlTrees$ = expanded$.pipe( |
There was a problem hiding this comment.
We don't need the urlTree$ variable here anymore. Can be converted to:
return expanded$.pipe(map(...), (catchError(...));
| expanded$, (rootSegmentGroup: UrlSegmentGroup) => | ||
| this.createUrlTree(rootSegmentGroup, tree.queryParams, tree.fragment !)); | ||
| return _catch.call(mapped$, (e: any): Observable<UrlTree> => { | ||
| const mapped$ = expanded$.pipe( |
There was a problem hiding this comment.
Same here:
return expanded$.pipe(map(...), (catchError(...));
There was a problem hiding this comment.
In fact, it's probably even easier to read as:
return this.expandSegmentGroup(this.ngModule, this.config, tree.root, PRIMARY_OUTLET)
.pipe(
map(...),
catchError(...)
);
And this saves needing to create the interim const variables. This applies above as well.
| if (e instanceof NoMatch) { | ||
| // TODO(i): this return type doesn't match the declared Observable<UrlSegmentGroup> - | ||
| // talk to Jason | ||
| return of (null) as any; |
There was a problem hiding this comment.
Remove as any & the comment (types should flow now).
| }); | ||
| return of (...routes).pipe( | ||
| map((r: any) => { | ||
| const expanded$ = this.expandSegmentAgainstRoute( |
There was a problem hiding this comment.
return this.expandSegmentAgainstRoute(...).pipe(catchError(...));
| import {map} from 'rxjs/operator/map'; | ||
| import {mergeMap} from 'rxjs/operator/mergeMap'; | ||
| import {Compiler, InjectionToken, Injector, NgModuleFactory, NgModuleFactoryLoader} from '@angular/core'; | ||
| // TODO(i): switch to fromPromise once it's expored in rxjs |
|
You can preview 8b0784e at https://pr22573-8b0784e.ngbuilds.io/. |
|
You can preview 246285b at https://pr22573-246285b.ngbuilds.io/. |
benlesh
left a comment
benlesh
left a comment
There was a problem hiding this comment.
Recommend this goes to beta.0 instead.
|
Hi @IgorMinar! This PR has merge conflicts due to recent upstream merges. |
… we update cli to v6
|
You can preview ecfdbaa at https://pr22573-ecfdbaa.ngbuilds.io/. |
angular-in-memory-web-api is not yet compatible with rxjs v6 and rxjs v6 backwards compatibility package is not yet ready to be used.
the next step will be to update all of aio to 6.0.0-rc.0
|
You can preview 6427180 at https://pr22573-6427180.ngbuilds.io/. |
|
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. |
Bam!
This PR is much bigger than typical RxJS code because of a unusual way we used RxJS in the past in order to reduce payload size and not introduce Observable.prototype patching due to Angular's use of RxJS.