GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — update to [email protected] by IgorMinar · Pull Request #22573 · angular/angular

Conversation

IgorMinar commented Mar 3, 2018

Copy link

Copy Markdown

Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

alan-agius4 reacted with thumbs up emoji

frederikprijck reacted with hooray emoji

All reactions

👍 1 reaction
🎉 1 reaction

IgorMinar changed the title ~~update to [email protected]~~ WIP: update to [email protected]

Mar 3, 2018

googlebot added the cla: yes label

Mar 3, 2018

IgorMinar requested review from jasonaden and vikerman

March 5, 2018 17:23

alexeagle added the area: packaging Issues related to Angular's creation of npm packages label

Mar 5, 2018

IgorMinar requested a review from bowenni

March 6, 2018 02:30

angular deleted a comment from mary-poppins

Mar 6, 2018

angular deleted a comment from mary-poppins

Mar 6, 2018

angular deleted a comment from mary-poppins

Mar 6, 2018

angular deleted a comment from mary-poppins

Mar 6, 2018

angular deleted a comment from mary-poppins

Mar 6, 2018

angular deleted a comment from mary-poppins

Mar 6, 2018

alfaproject reviewed

Mar 6, 2018

View reviewed changes

Comment thread

packages/bazel/src/ng_package/ng_package.bzl Outdated

alfaproject Mar 6, 2018

Copy link

Copy Markdown

Contributor

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.

Inconsistent quotes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

IgorMinar Mar 6, 2018

Copy link

Copy Markdown

Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

googlebot added cla: no and removed cla: yes labels

Mar 6, 2018

gkalpak force-pushed the rxjs/v6-alpha.3 branch 3 times, most recently from aed6612 to c0bf0ad Compare

March 6, 2018 17:01

angular deleted a comment from googlebot

Mar 6, 2018

angular deleted a comment from mary-poppins

Mar 6, 2018

angular deleted a comment from googlebot

Mar 6, 2018

angular deleted a comment from mary-poppins

Mar 6, 2018

angular deleted a comment from mary-poppins

Mar 6, 2018

angular deleted a comment from mary-poppins

Mar 6, 2018

angular deleted a comment from mary-poppins

Mar 6, 2018

angular deleted a comment from googlebot

Mar 6, 2018

angular deleted a comment from mary-poppins

Mar 6, 2018

angular deleted a comment from mary-poppins

Mar 6, 2018

angular deleted a comment from mary-poppins

Mar 6, 2018

IgorMinar commented

Mar 7, 2018

View reviewed changes

Comment thread

aio/content/examples/http/src/app/config/config.service.ts Outdated

IgorMinar Mar 7, 2018

Copy link

Copy Markdown

Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

benlesh Mar 7, 2018

Copy link

Copy Markdown

Contributor

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.

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))

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

IgorMinar Mar 8, 2018

Copy link

Copy Markdown

Contributor Author

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.

resolved via a fix of a bad import in rxjs

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mary-poppins commented Mar 13, 2018

Copy link

Copy Markdown

You can preview f22c86d at https://pr22573-f22c86d.ngbuilds.io/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mary-poppins commented Mar 13, 2018

Copy link

Copy Markdown

You can preview 1aea83c at https://pr22573-1aea83c.ngbuilds.io/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

There was an error while loading. Please reload this page.

There was an error while loading. Please reload this page.

jasonaden approved these changes

Mar 16, 2018

View reviewed changes

jasonaden left a comment

Copy link

Copy Markdown

Contributor

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.

Ignore comments for simplification changes. Will tack on to an add-on PR.

I removed a couple TODOs & pushed a fixup commit. Looks good!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

packages/router/src/apply_redirects.ts

		expanded$, (rootSegmentGroup: UrlSegmentGroup) => this.createUrlTree(
		rootSegmentGroup, this.urlTree.queryParams, this.urlTree.fragment !));
		return _catch.call(urlTrees$, (e: any) => {
		const urlTrees$ = expanded$.pipe(

jasonaden Mar 16, 2018

Copy link

Copy Markdown

Contributor

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 the urlTree$ variable here anymore. Can be converted to:

return expanded$.pipe(map(...), (catchError(...));

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

packages/router/src/apply_redirects.ts

		expanded$, (rootSegmentGroup: UrlSegmentGroup) =>
		this.createUrlTree(rootSegmentGroup, tree.queryParams, tree.fragment !));
		return _catch.call(mapped$, (e: any): Observable<UrlTree> => {
		const mapped$ = expanded$.pipe(

jasonaden Mar 16, 2018

Copy link

Copy Markdown

Contributor

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.

Same here:

return expanded$.pipe(map(...), (catchError(...));

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

jasonaden Mar 16, 2018

Copy link

Copy Markdown

Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

packages/router/src/apply_redirects.ts

		if (e instanceof NoMatch) {
		// TODO(i): this return type doesn't match the declared Observable<UrlSegmentGroup> -
		// talk to Jason
		return of (null) as any;

jasonaden Mar 16, 2018

Copy link

Copy Markdown

Contributor

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.

Remove as any & the comment (types should flow now).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

packages/router/src/apply_redirects.ts

		});
		return of (...routes).pipe(
		map((r: any) => {
		const expanded$ = this.expandSegmentAgainstRoute(

jasonaden Mar 16, 2018

Copy link

Copy Markdown

Contributor

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.

return this.expandSegmentAgainstRoute(...).pipe(catchError(...));

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

packages/router/src/router_config_loader.ts

		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

jasonaden Mar 16, 2018

Copy link

Copy Markdown

Contributor

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.

remove

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

mary-poppins commented Mar 16, 2018

Copy link

Copy Markdown

You can preview 8b0784e at https://pr22573-8b0784e.ngbuilds.io/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mary-poppins commented Mar 19, 2018

Copy link

Copy Markdown

You can preview 246285b at https://pr22573-246285b.ngbuilds.io/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

benlesh suggested changes

Mar 19, 2018

View reviewed changes

benlesh left a comment

Copy link

Copy Markdown

Contributor

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.

Recommend this goes to beta.0 instead.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ngbot Bot commented Mar 19, 2018

Copy link

Copy Markdown

Hi @IgorMinar! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gkalpak and others added 2 commits

March 19, 2018 17:56


          build(aio): temporarily use RxJS from root node_modules/ when using…

d488339

… local packages


          ci: temporarily increase payload size limit for cli-hello-world until…

59d4aa6

… we update cli to v6

mary-poppins commented Mar 20, 2018

Copy link

Copy Markdown

You can preview ecfdbaa at https://pr22573-ecfdbaa.ngbuilds.io/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Mar 20, 2018

fix(router): workaround issues in closure compiler #22870

Closed

google3 CI check seems to consider only the last commit and not the PR diff #22871

Closed

IgorMinar added 4 commits

March 19, 2018 21:13


          ci: temporarily disable offline_compiler_test.sh

43867b8


          ci: temporarily disable tests that depend on angular-in-memory-web-api

4413cf5

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.


          feat(core): upgrade rxjs to 6.0.0-alpha.4

b44560e


          build(aio): temporarily pin elements to 6.0.0-beta.8

the next step will be to update all of aio to 6.0.0-rc.0

mary-poppins commented Mar 20, 2018

Copy link

Copy Markdown

You can preview 6427180 at https://pr22573-6427180.ngbuilds.io/.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cexbrayat mentioned this pull request

Mar 20, 2018

fix(aio): fix errors after RxJS 6 migration #22886

Closed

1 of 3 tasks

jenniferfell mentioned this pull request

Mar 20, 2018

Verify docs match examples updates for [email protected] #22890

Closed

Maxim-Mazurok mentioned this pull request

Mar 3, 2019

fix(router): removed obsolete TODO comment #29085

Closed

3 of 14 tasks

angular-automatic-lock-bot Bot commented Sep 13, 2019

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Reviewers

3 more reviewers

alfaproject

alfaproject left review comments

benlesh

benlesh requested changes

jasonaden

jasonaden approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

action: merge

The PR is ready for merge by the caretaker

area: packaging

Issues related to Angular's creation of npm packages

cla: yes target: major

This PR is targeted for the next major release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.