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

refactor(QueryList): using ReplaySubject, emit results array from .ch… #9079

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@ajoslin
Contributor

ajoslin commented Jun 8, 2016

This is just a proposal to ask for feedback. Linked issue: #9078

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

…anges

@robwormald

This comment has been minimized.

Member

robwormald commented Jun 8, 2016

Oop, wrong account. LGTM once Travis goes green, and can you update the commit message to explain the change?

@ajoslin

This comment has been minimized.

Contributor

ajoslin commented Jun 8, 2016

I'll amend the PR with a proper BREAKING CHANGE footer, and a body explaining the change in detail.

I still need to write unit tests for it. I'm unable to get npm install working right now, though. Any tips?

@pkozlowski-opensource

This comment has been minimized.

Member

pkozlowski-opensource commented Jun 8, 2016

I'm unable to get npm install working right now, though. Any tips?

Node / npm version?

@ajoslin

This comment has been minimized.

Contributor

ajoslin commented Jun 8, 2016

I just read the contributing guide, looks like I need Node <6. I'll get that and follow up.

@ajoslin

This comment has been minimized.

Contributor

ajoslin commented Jun 8, 2016

I'm using Node 5.6.0 and npm 3.6.0. I was using Node 6.2.0 earlier.

npm install passed, but it looks like the developer guide is out of date. It references gulp test commands that only exist in gulpfile.js.old, not in the current gulpfile.js. How do I get tests to run?

@pkozlowski-opensource

This comment has been minimized.

Member

pkozlowski-opensource commented Jun 8, 2016

Yeh, dev guide is seriously out of date...

How do I get tests to run?

try ./test.sh

@ajoslin

This comment has been minimized.

Contributor

ajoslin commented Jun 8, 2016

Thanks, got 'em up and running. Getting some tests written now 👍

@ajoslin

This comment has been minimized.

Contributor

ajoslin commented Jun 8, 2016

Updated with passing tests and a fully written commit message.

ajoslin added a commit to ajoslin/angular that referenced this pull request Jun 8, 2016

feat(QueryList): immediately emit current results from .changes()
Closes angular#9079. Closes angular#9078.

(1) Instead of emitting no data, QueryList.prototype.changes now emits
its current array of results.
(2) QueryList.prototype.changes now immediately emits when `subscribe()`
is called.

BREAKING CHANGE: QueryList.prototype.changes will now emit immediately
on subscription, and pass its list of results into the `onNext` callback.

This means that the initial call to
`queryList.changes.subscribe((results) => {})` is all you need now to
handle the most up-to-date array of results.

ajoslin added a commit to ajoslin/angular that referenced this pull request Jun 8, 2016

feat(QueryList): immediately emit current results from .changes()
Closes angular#9079. Closes angular#9078.

(1) Instead of emitting no data, QueryList.prototype.changes now emits
its current array of results.
(2) QueryList.prototype.changes now immediately emits when `subscribe()`
is called.

BREAKING CHANGE: QueryList.prototype.changes will now emit immediately
on subscription, and pass its list of results into the `onNext` callback.

This means that the initial call to
`queryList.changes.subscribe((results) => {})` is all you need now to
handle the most up-to-date array of results.

ajoslin added a commit to ajoslin/angular that referenced this pull request Jun 8, 2016

feat(QueryList): immediately emit current results from .changes
Closes angular#9079. Closes angular#9078.

(1) Instead of emitting no data, QueryList.prototype.changes now emits
its current array of results.
(2) QueryList.prototype.changes now immediately emits when `subscribe()`
is called.

BREAKING CHANGE: QueryList.prototype.changes will now emit immediately
on subscription, and pass its list of results into the `onNext` callback.

This means that the initial call to
`queryList.changes.subscribe((results) => {})` is all you need now to
handle the most up-to-date array of results.

ajoslin added a commit to ajoslin/angular that referenced this pull request Jun 8, 2016

feat(QueryList): immediately emit current results from .changes
Closes angular#9079. Closes angular#9078.

(1) Instead of emitting no data, QueryList.prototype.changes now emits
its current array of results.
(2) QueryList.prototype.changes now immediately emits when `subscribe()`
is called.

BREAKING CHANGE: QueryList.prototype.changes will now emit immediately
on subscription, and pass its list of results into the `onNext` callback.

This means that the initial call to
`queryList.changes.subscribe((results) => {})` is all you need now to
handle the most up-to-date array of results.

ajoslin added a commit to ajoslin/angular that referenced this pull request Jun 8, 2016

feat(QueryList): immediately emit current results from .changes
Closes angular#9079. Closes angular#9078.

(1) Instead of emitting no data, QueryList.prototype.changes now emits
its current array of results.
(2) QueryList.prototype.changes now immediately emits when `subscribe()`
is called.

BREAKING CHANGE: QueryList.prototype.changes will now emit immediately
on subscription, and pass its list of results into the `onNext` callback.

This means that the initial call to
`queryList.changes.subscribe((results) => {})` is all you need now to
handle the most up-to-date array of results.
@ajoslin

This comment has been minimized.

Contributor

ajoslin commented Jun 8, 2016

It looks like it requires a change to public_api_spec since it's a breaking change, which needs approval from someone on the core team.

@robwormald

This comment has been minimized.

Member

robwormald commented Jun 8, 2016

you can go ahead and make the public API change to make the tests pass 👍

ajoslin added a commit to ajoslin/angular that referenced this pull request Jun 8, 2016

feat(QueryList): immediately emit current results from .changes
Closes angular#9079. Closes angular#9078.

(1) Instead of emitting no data, QueryList.prototype.changes now emits
its current array of results.
(2) QueryList.prototype.changes now immediately emits when `subscribe()`
is called.

BREAKING CHANGE: QueryList.prototype.changes will now emit immediately
on subscription, and pass its list of results into the `onNext` callback.

This means that the initial call to
`queryList.changes.subscribe((results) => {})` is all you need now to
handle the most up-to-date array of results.
@ajoslin

This comment has been minimized.

Contributor

ajoslin commented Jun 8, 2016

Done. 👍

@robwormald

This comment has been minimized.

Member

robwormald commented Jun 9, 2016

@ajoslin can you fix the implicitAny errors please? That changed overnight, sorry :octocat:

ajoslin added a commit to ajoslin/angular that referenced this pull request Jun 10, 2016

feat(QueryList): immediately emit current results from .changes
Closes angular#9079. Closes angular#9078.

(1) Instead of emitting no data, QueryList.prototype.changes now emits
its current array of results.
(2) QueryList.prototype.changes now immediately emits when `subscribe()`
is called.

BREAKING CHANGE: QueryList.prototype.changes will now emit immediately
on subscription, and pass its list of results into the `onNext` callback.

This means that the initial call to
`queryList.changes.subscribe((results) => {})` is all you need now to
handle the most up-to-date array of results.
@ajoslin

This comment has been minimized.

Contributor

ajoslin commented Jun 10, 2016

Not a problem. Updated.

@mhevery

This comment has been minimized.

Member

mhevery commented Jun 10, 2016

@ajoslin looks like it needs rebase again. @robwormald please add LGTM so that we can have your approval.

@robwormald

This comment has been minimized.

Member

robwormald commented Jun 10, 2016

its LGTM once conflicts are fixed (sorry, @ajoslin, uh, again) 👍

ajoslin added a commit to ajoslin/angular that referenced this pull request Jun 10, 2016

feat(QueryList): immediately emit current results from .changes
Closes angular#9079. Closes angular#9078.

(1) Instead of emitting no data, QueryList.prototype.changes now emits
its current array of results.
(2) QueryList.prototype.changes now immediately emits when `subscribe()`
is called.

BREAKING CHANGE: QueryList.prototype.changes will now emit immediately
on subscription, and pass its list of results into the `onNext` callback.

This means that the initial call to
`queryList.changes.subscribe((results) => {})` is all you need now to
handle the most up-to-date array of results.
@ajoslin

This comment has been minimized.

Contributor

ajoslin commented Jun 10, 2016

Rebased and updated.

ajoslin added a commit to ajoslin/angular that referenced this pull request Jun 10, 2016

feat(QueryList): immediately emit current results from .changes
Closes angular#9079. Closes angular#9078.

(1) Instead of emitting no data, QueryList.prototype.changes now emits
its current array of results.
(2) QueryList.prototype.changes now immediately emits when `subscribe()`
is called.

BREAKING CHANGE: QueryList.prototype.changes will now emit immediately
on subscription, and pass its list of results into the `onNext` callback.

This means that the initial call to
`queryList.changes.subscribe((results) => {})` is all you need now to
handle the most up-to-date array of results.

ajoslin added a commit to ajoslin/angular that referenced this pull request Jun 10, 2016

feat(QueryList): immediately emit current results from .changes
Closes angular#9079. Closes angular#9078.

(1) Instead of emitting no data, QueryList.prototype.changes now emits
its current array of results.
(2) QueryList.prototype.changes now immediately emits when `subscribe()`
is called.

BREAKING CHANGE: QueryList.prototype.changes will now emit immediately
on subscription, and pass its list of results into the `onNext` callback.

This means that the initial call to
`queryList.changes.subscribe((results) => {})` is all you need now to
handle the most up-to-date array of results.
@ajoslin

This comment has been minimized.

Contributor

ajoslin commented Jun 10, 2016

Pushed again after running clang-format.

ajoslin added a commit to ajoslin/angular that referenced this pull request Jun 11, 2016

feat(QueryList): immediately emit current results from .changes
Closes angular#9079. Closes angular#9078.

(1) Instead of emitting no data, QueryList.prototype.changes now emits
its current array of results.
(2) QueryList.prototype.changes now immediately emits when `subscribe()`
is called.

BREAKING CHANGE: QueryList.prototype.changes will now emit immediately
on subscription, and pass its list of results into the `onNext` callback.

This means that the initial call to
`queryList.changes.subscribe((results) => {})` is all you need now to
handle the most up-to-date array of results.
@ajoslin

This comment has been minimized.

Contributor

ajoslin commented Jun 11, 2016

Rebased and updated again. There was still one formatting error that I just fixed.

feat(QueryList): immediately emit current results from .changes
Closes #9079. Closes #9078.

(1) Instead of emitting no data, QueryList.prototype.changes now emits
its current array of results.
(2) QueryList.prototype.changes now immediately emits when `subscribe()`
is called.

BREAKING CHANGE: QueryList.prototype.changes will now emit immediately
on subscription, and pass its list of results into the `onNext` callback.

This means that the initial call to
`queryList.changes.subscribe((results) => {})` is all you need now to
handle the most up-to-date array of results.
@ajoslin

This comment has been minimized.

Contributor

ajoslin commented Jun 11, 2016

My tests are now failing on Travis in Chrome, but I can't get them to fail locally. Any ideas?

@robwormald

This comment has been minimized.

Member

robwormald commented Jun 21, 2016

looking into this now.

@ajoslin

This comment has been minimized.

Contributor

ajoslin commented Jun 21, 2016

Find anything? Would another rebase help?

@@ -30,9 +31,9 @@ import {getSymbolIterator} from '../facade/lang';
export class QueryList<T> {
private _dirty = true;
private _results: Array<T> = [];
private _emitter = new EventEmitter();
private _emitter = new ReplaySubject<Array<T>>(1);

This comment has been minimized.

@tbosch

tbosch Jun 29, 2016

Member

This will tie Angular core more closely to Rx.
Could we implement this manually?

The logic is simple: Whenever a subscriber subscribes and this._dirty === false, emit this._results. If this._dirty === true, we don't need to emit on subscription, as Angular will emit during the next change detection run itself.

@vicb

This comment has been minimized.

Contributor

vicb commented Nov 10, 2016

Closing for now - until @robwormald unveils his master plan for reactive angular.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment