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

Support cold event streams in templates #13248

Open
mhevery opened this Issue Dec 5, 2016 · 40 comments

Comments

Projects
@mhevery
Copy link
Member

mhevery commented Dec 5, 2016

Requirements

  • [R1] Observables must be cold (ie if there is no subscription then there should be no listeners.)
    USE CASE: drag and drop. No need to listen on mousemove if drag has not been initiated.
  • [R2] It must be possible to execute an expression which computes the action which will be next-ed into the observable.
    • USE CASE: ngFor shows a list of todos with a remove button. Clicking the remove should notify the stream not with the DOM event, but with the item to be removed.
  • [R3] It must be possible to set up observable recipes to run inside/outside of angular zone.
    • USE CASE:
      • drag and drop. Don't run CD on mousemove.
      • Throttle; debounce; etc..: Only run CD on subscription which pass throttle, debounce; etc.
  • [R4] It would be strongly preferable if there was something in the template which would indicate that an observable is listening at a given location.
    • USE CASE: it should be easy for the developer/tooling to see where in the source code the event will be processed.

Design

  • Component will annotate fields which will represent Observable Streams which will be created by the Template's ViewFactory. [R4] (This will also determine the parameterization for the Observables. (ie Observable<MyType>))
    • The Framework will set those fields right after constructing the object. We need a new hook (ngOnCreate) which would be called right after setting the fields but before running first change detection (ngOnChanges, ngOnInit).
  • Create new syntax which will describe [R4]:
    • Which events should the template listen on [R4].
    • How should the action be computed [R2].
    • Which stream should receive the action
      *Split Angular Zone into NgErrorZone for error handling and NgChangeDetectionZone for CD. [R3]
    • Provide convenience method running code in NgErrorZone and NgChangeDetectionZone. [R3]
@Component({
  template: `
    <ul>
      <li *ngFor="let user of users; let i = index">
        {{user.name}}
        <button (*click|remove)="{item: user, index: i}">X</button>
      <li>
    <ul>
`
})
class MyComponent implements OnCreate{
  users: User[] = ...;


  // Specify that this will have property injection.
  @Inject()
  remove: Observable<{item: User, index: number}>;


  actions: Observable<Action>;


  ngOnCreate() {
    // An example of building up a recipe.
    // Notice that we are making sure that Rx operators have no CD.
    this.actions = noChangeDetection(() => remove.map(() => ...));
  }
}
  • ViewFactory creates an instance of Observable which then is property injected into component
    • Observable names and types (Type parameters) are declared in the component
    • @Inject() identifies that the observable has to be generated in the ViewContainer.
    • Need a ngOnCreate() hook to create recipes.
  • Need noChangeDetection() method for going in and out of CD. Separate Design Doc TBD.
  • (*click|remove)="{item: user, index: i}" Placeholder syntax, see discussion later. This is used to generate cold event registration for DOM events. The expression is used to compute the value which will be next ed into the observable.
    • Notice that even though there are zero or more instance of places where we listen into DOM events, there is always only one observable instance given that name.

Syntax Discussion

NOTE: The above syntax is used only as a placeholder

The Syntax Requirements:

  • The syntax needs to encode three items:
    • event: The DOM event
    • stream: The destination stream
    • actionExp: The action object
  • Given that we need to encode three things and that key="value" only takes two by default we have to figure out how to encode three things into two. The choices are:
    • event+stream="actionExp": [favorite]
      • This format combines event and stream on the LHS.
      • Both event and stream are just symbols (ie foo) not complex expressions (ie {name: foo + 2}) and so they can be on the LHS.
      • Event is already on LHS in (event)="stmt" so it should stay there.
      • There is already a precedence for compound LHS in style.width.px="123".
      • Example: (*click|remove)="actionExp".
        • The * is preferred (not necessary) as it hints that it is cold (not always listening)
        • The (...) should be retained to be consistent with normal event listeners
    • event="stream+actionExp": [probably not]
      • Micro Syntax has precedence for combining multiple formats on the RHS (but it requires special symbol on the LHS to activate it.)
      • Microsyntax is shorthand for an expanded canonical form. Here there would be no canonical form.
      • Example: (*click)="actionExp into remove"
        • The * is required as it hints that it is cold (not always listening) and that it is different from normal listener.
        • The (...) should be retained to be consistent with normal event listeners
  • stream-action="actionExp" stream-event="event": [strongly against]
    • Require two key-value pairs.
    • This format is just very wordy and cumbersome.
    • Puts event on the RHS whereas it is on LHS in (event)="stmt".
@frederikschubert

This comment has been minimized.

Copy link

frederikschubert commented Dec 5, 2016

Why can't we reuse the | combinator? As in (event)="actionExp | stream".
This would then read:
In case of event X, pipe this Object into that stream. The concept of a pipe is pretty similar to an observable and the syntax fits nicely into the existing template syntax IMO.

@lacolaco

This comment has been minimized.

Copy link
Contributor

lacolaco commented Dec 6, 2016

For the parser (and humans), I think a specific prefix symbol is needed.
AFAIK, RxJS users prefer $ character to indicate that is $(s)tream. So my idea is the below

  • <button ($click+remove)="{item: user, index: i}">X</button>
@flga

This comment has been minimized.

Copy link

flga commented Dec 6, 2016

The most useful thing (for the developer) about expressing events as streams is the ability to control the flow of events using operators like debounce etc.
To accomplish this all we need to do is create an emitter (Subject) to capture those events.

Something like

@Component({
  template: `
    <ul>
      <li *ngFor="let user of users; let i = index">
        {{user.name}}
        <button (click)="remove$.next({item: user, index: i})">X</button>
        <button (click=>remove)="remove$.next({item: user, index: i})">X</button>
      <li>
    <ul>
    `
})
class MyComponent implements OnInit {
  users: User[] = ...;
  remove$: Subject<{item: User, index: number}>;
  actions$: Observable<Action>;

  ngOnInit() {
    // An example of building up a recipe.
    // Notice that we are making sure that Rx operators have no CD.
    this.actions$ = noChangeDetection(() => this.remove$.map(() => ...));
  }
}

That said, there is indeed an opportunity to decrease the number of listeners by having lazyness, therefore reducing memory pressure.

As we know the same behaviour can be achieved with manual wiring up of event listeners using Observable.fromEvent() so the syntax has to be more convenient than manual wiring, otherwise it's not worth it.

The most familiar way would be to have something like:
<button (click$remove)="{item: user, index: i}">X</button>:

A hot stream:
<button (click$remove.hot)="{item: user, index: i}">X</button>
and a cold one:
<button (click$remove.cold)="{item: user, index: i}">X</button>
<button (click$remove)="{item: user, index: i}">X</button> (implicitly cold)

I think that is what conforms best with the current syntax, however I find that this would be much more terse:
<button (click$)="remove({item: user, index: i})">X</button>
But, it would probably require special casing the parser, doesn't seem viable.

If the microsyntax without a macrosyntax issue turns out not to be a problem then maybe this:
<button ($click)="into remove using {item: user, index: i}">X</button>
Which is verbose but explicit, a more terse option:
<button ($click)="{item: user, index: i} into remove">X</button> EDIT: oops, just noticed this is exactly the same as you proposed

@vicb

This comment has been minimized.

Copy link
Contributor

vicb commented Dec 6, 2016

Some thoughts on the proposed syntax:

  • 👎 on @Inject() - "why can we Inject anything in the ctor but not in a property" ? Different mechanisms need different name
  • 👎 on the stream name on the LHS - non stream syntax is (event)="action()", RHS seems more consistent in this sense.

What about (*event)="value >> obs" - The * is preferred (not necessary) as it hints that it is cold (not always listening)

(btw, I don't get why the * hints it's cold ? could you expand ?)

@vicb

This comment has been minimized.

Copy link
Contributor

vicb commented Dec 6, 2016

@mhevery could you please turn this issue into a proper spec (google docs) ? and keep this issue updated with the change log of the doc.

The doc should contain:

  • Issue statement,
  • Reqs,
  • Proposal,
  • Alternate proposals with pros and cons,
  • Syntax,
  • Use cases (implementation with the proposal),
  • Change log.

I think it will be easier to discuss that way

@mhevery

This comment has been minimized.

Copy link
Member Author

mhevery commented Dec 7, 2016

Just leaving for the record here all of the syntax forms which we have considered:

<input #a (keyup)="$event >> changes">
{{a.value}}

<ol *ngFor="let item of items">
<template ngFor let-item [ngForOf]="items">
  <button (*click|action)="{action: remove, item: item}" let-foo="..">
  <button stream-click-into-action="{action: remove, item: item}">
  
  <my-component (*open|action)="{action: 'open', item: $event}">

  <button (click)="{action: remove, item: item} >> action">
  <button (click)="{action: remove, item: item}">
  <button (*click)="{action: remove, item: item} >> action">
  <button (*click)="action << {action: remove, item: item}">
  <button (*click)="{action: remove, item: item} >> action().foo">
  
  
  <button (*click)="stream action << {action: remove, item: item}">
  <button (*click|action)="{action: remove, item: item}">
  <button on-click-into-action="{action: remove, item: item}">
  <button stream-click="{action: remove, item: item} >> action">
  <button stream-click="stream action action << {action: remove, item: item} >> action">
  <button (!click|action)="{action: remove, item: item}">
  <button {click|action}="{action: remove, item: item}">

  <button {click}="action << observable$.map(() => {do: remove, item: item}).debounce()">
  
class MyComp {
@Stream???()
action: Observable<{action:string, item: any}>;

}

foo() {}







<input #a (change)="exp()" (*keyup)="foo(); a.value >> change" >
<div (mousemove)="dragging ? a >> move : b"
     (*mousemove|onMove)="$event"
{{a.value}}



  
  
@Inject()
Stream<Item> removeStream

@mhevery mhevery changed the title Support cold DOM event streams in templates Support cold event streams in templates Dec 13, 2016

@mhevery

This comment has been minimized.

Copy link
Member Author

mhevery commented Dec 16, 2016

@vicb and I had a brainstorming session and came up with this syntax. /cc @IgorMinar @Blesh .

<button (*click)="streamExpression >> dstStream">...</button>

Where:

  • dstStream is a property of the controller object which receives the stream.
  • streamExression is something which adds operators to a stream such as $stream.map(...).debounce()

A common usage may be this:

@Component({
  template: 
    `<button (*click)="$stream.map(()=> {action:'add', text: 'test'}) >> action">test</button>`
})
class MyComponent {
  @Stream()
  action: Observable<{action: string}>;
}

The above syntax is a bit verbose, so we will have a special map syntax as it is the most used primitive with Observables.

<button (*click|action)="{action:'add', text: 'test'}">test</button>

It is important to understand when the RHS of the expression executes in the component lifecycle.

Given:

<ul>
  <li *ngFor="let item of items">
    <button (*click)="$stream.debounce(10).map(() => {action: 'REMOVE', item: item) >> action">remove</button>
  </li>
</ul>
<button (*click|action)="{action: 'ADD', text: 'test'}">add</button>

It is important to understand that the expression associated with (*click) will execute exactly once at the beginning of the template creation as the *ngFor is unrolled. This means that the only way to read the value item is through a closure, because the item can change at later point in time, and we want the current value of item not initial value of item

NOTE: (Closure syntax would have to be added to the parser)

Pipes

The advantage of this syntax is that one cane use pipe and custom function to build up stream recipes such as:

<button (*click)="myFn($stream) | debounce:100 >> action">test</button>
@mhevery

This comment has been minimized.

Copy link
Member Author

mhevery commented Dec 16, 2016

Unifying cold/hot streams.

@Component({
  template: `
    <!-- What can be done today (But event is hot registered)-->
    <button (click)="action.next($event)">
    <!-- Would be equivalent to simply piping `$stream` into `action`.-->
    <!-- But in this example the DOM event is registered only if a stream has listener -->
    <button (*click|action)="$stream">
    <!-- Which can be expressed in the short form as this -->
    <button (*click|action)>


    <!--  More important use case is nexting a value other then an event. -->
    <button (click)="action.next({action: 'ADD', value: item})">
    <!--  Which is the same as using the map operator on the observable. -->
    <button (*click|action)="$stream.map(() => {action: 'ADD', value: item})">
    <!--  Which can be simplified for the common form using AST transforms as such. -->
    <button (*click|action)="map: {action: 'ADD', value: item}">
  `
})
class MyComponent {
  // We instantiate the EventEmitter synchronously
  action: Observable<any> = new EventEmitter();

  // which then allows building up recipes like so
  removes = () => this.action.map(...));
}

Under the hood the when we compile a template:

<button (*click|action)="$stream.map(() => {action: 'ADD', value: item})">

It generates something like so:

////// GENERATED
  
ngOnViewInsert() {
  this.click1$ = $stream.map(() =>  {action: 'ADD', value: item});
  this.action.addStream(this.click1$);
}
  
ngOnViewRemove() {
  this.action.removeStream(this.click1$);  
}
@benlesh

This comment has been minimized.

Copy link
Contributor

benlesh commented Dec 17, 2016

My only concern with putting too much of your observable configuration in the view/template would be that it's going to be much harder to test. What if that set up was done in the component?

@Component({
  template: `
    <button (*click|action)="setupClicks">Click Me</button>
  `
})
class MyComponent {
  action: Observable<ClickAction> = null;

  i = 0;

  setupClicks(click$: Observable<MouseEvent>) {
    return click$.map(e => ({ type: 'CLICK_ACTION', value: this.i++ });
  }
}

This way you can unit test that setup function.

Seems like just as much boilerplate as anything though.

@vicb

This comment has been minimized.

Copy link
Contributor

vicb commented Dec 17, 2016

@Blesh that would be supported with the proposed syntax:

@Component({
  template: `<button (*click|action)="setupClicks($stream)">Click Me</button>`
})

You want to put the logic in your template when you access context specific info (think loop item, index in a NgFor) which are not available on your component instance.

@wardbell

This comment has been minimized.

Copy link
Contributor

wardbell commented Dec 17, 2016

I am intrigued but, I think like @Blesh, I find so much syntax in the html to be smelly. Testing is one concern but it goes way beyond that.

Maybe when I read an earlier part of this thread I'll understand what use cases make coding the template so much better than coding in the component. We will certainly have to articulate that to a skeptical world.

@mhevery

This comment has been minimized.

Copy link
Member Author

mhevery commented Dec 17, 2016

Self Note:

Turns out that we can't use foo: as prefix for Ast transform as it would collide with macro-syntax. For example if we had:

[tooltip]=" 'text', height: 10 "

which would become

[tooltip]="foo: 'text', height: foo: 10"

Which would create an ambiguity, as it would not be clear if we are binding to tooltip with foo AST transform or binding to tooltipFoo with no AST transform. So we need a different syntax. Perhaps something like foo:: which would then become

[tooltip]="foo::'text' , height: foo::10 "

or in the case of stream:

<button (*click|action)="map::{action: 'ADD', value: item}">

However it may be the case that the only a part of the overall AST may be transformed, so maybe the above syntax is too restrictive as it only allows the whole template to be transformed.

@fxck

This comment has been minimized.

Copy link

fxck commented Dec 19, 2016

AFAIK, RxJS users prefer $ character to indicate that is $(s)tream. So my idea is the below

Yes, they do, but appended, not prepended, $whatever usually means jquery(!!!!!) object, while whatever$ is usually a stream.

Why is it not?

(click$|action)

For me, * indicates structural directive, absolutely nothing to do with streams or observables.

Also where is @robwormald and @MikeRyan52 in this discussion?

@kylecordes

This comment has been minimized.

Copy link

kylecordes commented Dec 19, 2016

I like to echo several others here with a recommendation to keep the template syntax minimal, just enough hook to wire up to the plain old TypeScript code in the class, keep the complexity over there.

@benlesh

This comment has been minimized.

Copy link
Contributor

benlesh commented Dec 19, 2016

It seems to me like something more minimalistic might work, something like:

@Component({
  template: `
    <input #q (keypress)="({ action: 'FOO', value: q.value }) -> qChange$"/>
    <div *ngFor="let result of searche$ | async">{{result.text}}</div>
  `
})
class MyComponent {
  qChange$: Observable<string>; 

  get searche$(): Observable<SearchResult[]> {
    this.qChange$.debounceTime(500)
      .switchMap(query =>
        http.get('/search/' + query)
          .map(res => res.json())
      )
  }
}

The -> operator would be of the lowest possible precedence so <stuff here> in <stuff here> -> observable$ would be processed first. It could even be something like qChange$(<stuff here>), which might look more idiomatic to JS developers.

I really don't think it's a great idea to put too much in the way of composed operators in the template. For one thing, testing anything async that happens in there will become more difficult. A debounce, for example.

However, I can totally see why it would be beneficial to have some cleaner way to push into a cold observable.

The advantage I can see to a -> operator is it becomes more grep-able.

@mhevery

This comment has been minimized.

Copy link
Member Author

mhevery commented Dec 20, 2016

@Blesh sorry but (keypress) would mean that the registration is hot. We need different semantics for cold registration.

BTW, this works today.

<input #q (keypress)="action.next({action: 'FOO', value: q.value })"

@mhevery

This comment has been minimized.

Copy link
Member Author

mhevery commented Dec 20, 2016

Why is it not?

(click$|action)

It would be on the stream which has $ not event, since one could have a component with such an event. So it would be (click|action$) if you name your stream that way. (That is your choice not a framework forcing you.)

the reason for * is that we need some sort of prefix so that IDE tooling can work better. You want to notify the IDE before you autocomplete not after. It needs to be a char which could not be in variable name. So $ or _ is out.

@benlesh

This comment has been minimized.

Copy link
Contributor

benlesh commented Dec 20, 2016

@Blesh sorry but (keypress) would mean that the registration is hot.

@mhevery why is keypress hot, but click is not? Presumably they're both event streams registered in the same manner against a DOM element that doesn't exist until it's mounted to the view. Or do you mean I should put an * up front or something to signify that it's an event that isn't processed until mount-time. like (*keypress)=""?

@vicb

This comment has been minimized.

Copy link
Contributor

vicb commented Dec 20, 2016

@Blesh because (keypress)="..." is an existing syntax in ng2 and uses addEventListener() while (*click|dst)="..." would be a new Angular construct and would use Observable.fromEvent().

I think this GH issue mixes two concerns:

  • how to express stream transformations, ie (*src|dst)="$stream.trans1().trans2()",
  • the fact that src streams built from DOM events should be cold - which I think nobody here disputes.
@kylecordes

This comment has been minimized.

Copy link

kylecordes commented Dec 20, 2016

How about the following alternative, even less new syntax than described by @Blesh?

In this approach, we would use the existing # to mark an element, and the existing () syntax to capture an event, but leave out the value for the attribute.

Although there seems to be a lot of enthusiasm here for lots of new syntax to use inside a template, overall I believe we are mostly better served by not introducing more new syntax or more kinds of expressions into the template language. We already have a very serviceable language, TypeScript, with a very serviceable way to annotated with extra bits that don't fit in the language itself.

@Component({
  template: `
    <input #foo (change) >
    <div *ngFor="let result of searche$ | async">{{result.text}}</div>
  `
})
class MyComponent {
  @Stream('foo', 'change')
  fooChange$: Observable<string>; 

  get searche$(): Observable<SearchResult[]> {
    this.fooChange$.debounceTime(500)
      .map(e => e.value)
      .switchMap(query =>
        http.get('/search/' + query)
          .map(res => res.json())
      )
  }
}
@benlesh

This comment has been minimized.

Copy link
Contributor

benlesh commented Dec 20, 2016

@vicb Then I'd stick with (*src | propName) or any other forward-compatible syntax and leave the transformation out of the proposal for now. /@mhevery

@PatrickJS

This comment has been minimized.

Copy link
Member

PatrickJS commented Dec 21, 2016

  • Can we use ^ rather than * since teaching * microsyntax is already confusing enough.

  • Rather than @Stream can we call it @ObservableEvent or something along those lines.

  • Can we also change ngOnCreate a bit to be invoked before setting the event values so

  @Input() value = DEFAULT_VALUE;
  ngOnCreate(observableInputs) {
    console.log(this.value) // DEFAULT_VALUE
  }
  • I need this for Universal to overwrite the default values during server/client hydration
  constructor(public store: Store) {
  }
  @Input() value = DEFAULT_VALUE;
  ngOnCreate(observableInputs) {
    console.log(this.value) // DEFAULT_VALUE
    this.value = this.store.get('HomeComponent.value', this.value);
    // angular bootstraps with the batch data made on the server which allows
    // the client to load faster since only one http call was made (fetching index.html)
  }
  • I also need something like observableInputs to reproduce "mini Redux" from my prototype made in this comment #4062 (comment) where we need the initial value via an observable to correctly set the initial value.
  @Output() counterChange = Observable.merge(
      this.increments,
      this.decrements.mapTo(-1)
    )
    .scan((total, value) => total + value, 0); // notice the initial value is hard coded to 0 rather than using observableInputs
  // using @ObserveInput() was the solution but it's hacky since this was mostly
  // a proof of concept to show that we need this feature
  • We can also expand the arguments for ngOnCreate which would lead to more CycleJS design (which is great)
  ngOnCreate(componentLifeCycleRef: ComponentLifeCycleRef) {
    componentLifeCycleRef.inputs.value$ // object of input observable for the above examples
    componentLifeCycleRef.content$ // hook into values after content is init
    componentLifeCycleRef.view$ // hook into values after view is init
    componentLifeCycleRef.onDestory$ // we can now use takeUntil so we can avoid using ngOnDestory to unsub
    componentLifeCycleRef.onChanges$ // we can hook into cd
  }

now that is Extremely Powerful if you also think of Reactive Forms and the Reactive Router all providing Observables.

PS: Also keep in mind React is also running into problems with their lifecycle hooks for their Fiber engine that is avoided with ComponentLifeCycleRef and ngOnCreate.

(see image below)

In Fiber, the engine is in control of scheduling when something should be rendered during one frame or the next to ensure 60fps (much like a game engine) which means having side-effects is not a great idea. They may cancel a component before data which would lead to side effects if a user does something in constructor or componentWillMount. To fix this they want users to use two lifecycle hooks (componentDidMount our ngOnInit which is after data and componentWillUnmount our ngOnDestroy) to create their subscriptions and unsubscriptions rather than using the constructor or componentWillMount. That is great but it has problems with server-rendering as you need the data synchronously (to overwrite default data but not new data from inputs/props) you may also have your own state solution that would like to provide defaults values that would also conflict since this is ran after initial data (which usually means new data rather than default data). For Angular Universal, we use the constructor to run this logic but can also do the same logic with ngOnChanges checking for undefined to determine if it's before CD to set the updated default values before any new data from Input. Since Angular is using RxJS we only need to include one Lifecycle hook ngOnCreate which should include something along the lines of ComponentLifeCycleRef, mentioned above, that allow us to do stuff like .takeUntil to avoid using another lifecycle hook to clean up taking advantage of cold observables to "destroy" the component and data flow before CD if we so happen to need such a feature

screen shot 2016-12-21 at 8 24 32 am

PPS: There seems to be a trend of wanting to do something with eventual values for example QueryList, Input, RouteParams where you want to wire up what happens when you get the value (#9079). Adding ngOnCreate would allow anyone to wire up the whole component's data flow in one function much like CycleJS and would complete the story of having the developer write async code (rxjs) via ngOnCreate or sync code (use lifecycle hooks) via ngOnDestroy/ngAfterViewInit/ngOnInit/ngAfterContentInit

github-tipe-logo

@mattpodwysocki

This comment has been minimized.

Copy link

mattpodwysocki commented Dec 21, 2016

@gdi2290 why is 0 hard coded as a seed as it won't be used since you're already using startWith(this.counter)?

@PatrickJS

This comment has been minimized.

Copy link
Member

PatrickJS commented Dec 21, 2016

(update: I removed .startWith() in the example in the previous comment)
@mattpodwysocki 0 is hard coded because we need an Observable of the Inputs (I hardcoded it because the example would look a bit more complex, see below). So the example was showing what we could do while the correct solution would look a bit weird without better framework support, for example using angular2-observe-decorators

  @ObserveInput() initialValue$ = new EventEmitter();
  @Output() counterChange = this.initialValue$
    .mergeMap(initialValue => {
      return Observable.merge(
        this.increments,
        this.decrements.mapTo(-1)
      )
      .scan((total, value) => total + value, initialValue);
    });

the observe-decorators create a proxy (Subjects) that will emit the values when they are available later in the lifecycle of the component. This will allow us to wire up the data flow when we create the component but it's a bit awkward. Having ngOnCreate that provided an ObservableComponentRef with Observables would help with this so we can do stuff like .takeUntil or grab the initial values

github-tipe-logo

@PatrickJS

This comment has been minimized.

Copy link
Member

PatrickJS commented Dec 21, 2016

Another great thing about having ngOnCreate is that the separation of concerns for getting data in a component is more intuitive. Right now users are assuming they should grab data in the constructor which makes it hard to test and feels awkward.

// actual code used somewhere in a codebase far far away
  constructor(public http: Http) {
    this.http.get('data.json')
     .map(res => res.json())
     .toPromise() // yes, this happens lol
     .then(data => this.data = data);
  }

If we simply suggest users to use ngOnCreate for data then any future refactors would make a lot more sense. For example refactoring the data fetching to the router then by the time you inject the router in the component you will have the initial data wouldn't be too hard refactoring in ngOnCreate. Adding any sort of Relay like state management would also need to use ngOnCreate to set the initial values and to trigger change detection or not via helper function noChangeDetection/changeDetection (think $apply)

github-tipe-logo

mhevery added a commit to mhevery/rxjs that referenced this issue Jan 8, 2017

feat(Zone): add support for Zone.js
Add support for Zone.js in a way which should have minimal impact on performance.

Ensures that all callbacks are invoked in the same `Zone` as the `Zone` which was current at
the time of the callback registration with the `Rx`.

-  Add `getZone()` method which returs always `null` if no `Zone` is present or returrns
   the current zone.
 - All places where the `Zone` needs to be intered are quarded to only go through zone if the
   Zone is different from the current `Zone` for performance reasons.

You can read up on Zones in [The Zone Primer](https://docs.google.com/document/d/1F5Ug0jcrm031vhSMJEOgp1l-Is-Vf0UCNDY-LsQtAIY).
Before this change when Rx runs in the Zone environment it will
propagate Zones according to VM Tasks. In the case of Rx this is not
correct behavior because Rx does its own scheduling and the result is
that all callbacks end up running in the same zone as Task zone. This
behavior is mostly correct most of the time, but there are cases which
result in wrong behavior which this change corrects. This change is
needed to enable proper implementation of
angular/angular#13248.

mhevery added a commit to mhevery/rxjs that referenced this issue Jan 9, 2017

feat(Zone): add support for Zone.js
Add support for Zone.js in a way which should have minimal impact on performance.

Ensures that all callbacks are invoked in the same `Zone` as the `Zone` which was current at
the time of the callback registration with the `Rx`.

-  Add `getZone()` method which returs always `null` if no `Zone` is present or returrns
   the current zone.
 - All places where the `Zone` needs to be intered are quarded to only go through zone if the
   Zone is different from the current `Zone` for performance reasons.

You can read up on Zones in [The Zone Primer](https://docs.google.com/document/d/1F5Ug0jcrm031vhSMJEOgp1l-Is-Vf0UCNDY-LsQtAIY).
Before this change when Rx runs in the Zone environment it will
propagate Zones according to VM Tasks. In the case of Rx this is not
correct behavior because Rx does its own scheduling and the result is
that all callbacks end up running in the same zone as Task zone. This
behavior is mostly correct most of the time, but there are cases which
result in wrong behavior which this change corrects. This change is
needed to enable proper implementation of
angular/angular#13248.
@fxck

This comment has been minimized.

Copy link

fxck commented Jan 11, 2017

Any development on this? Looks like it's blocked on rxjs support for zonejs or something?

edit: ReactiveX/rxjs#2266 good read

@LinBoLen

This comment has been minimized.

Copy link

LinBoLen commented Jan 12, 2017

more clear right now

@mitCode

This comment has been minimized.

Copy link

mitCode commented Mar 15, 2017

Is this going to be available in Angular 4 or is it put off to a future time? Couldn't find any info about it.

@DzmitryShylovich

This comment has been minimized.

Copy link
Contributor

DzmitryShylovich commented Mar 15, 2017

put off to a future time

@fxck

This comment has been minimized.

Copy link

fxck commented Apr 25, 2017

Any news on this @mhevery ? I heard some rumors, but it would be nice to have an official update.

@wis

This comment has been minimized.

Copy link

wis commented Jun 4, 2017

Bump!
Go ahead thumbsdown vote/react 😆

Jk aside, I think at least sometimes Engineers should prioritize features & projects that help with Google's revenue, like this feature & the AngularFire2 project. synergy between Angular and Firebase projects/teams should be encouraged.

apologies for bothering, if the Core Roadmap with issues like this one are blocking the way, and that's why it's put off to a future time.

@fxck

This comment has been minimized.

Copy link

fxck commented Oct 6, 2017

Hi, @robwormald, @mhevery could you please give an update on this?

They have a much simpler proposal now
They are going to be adding an on method to ElementRef that allows you to hook up a cold observable
It is great because it will work for host elements, view queries, and content queries
They will use it as a building block for better reactive APIs down the road.

This is the rumor I heard, six months ago.

@LinBoLen

This comment has been minimized.

Copy link

LinBoLen commented Oct 15, 2017

I don't think it will be implemented currently. programming on template relate to rxjs?
I agree with angular's decision. new concepts need time to digest.

@fxck

This comment has been minimized.

Copy link

fxck commented Dec 14, 2017

whatwg/dom#544 apparently this is a thing. But hey, why tell us, it's not like anyone cares.

@mhevery mhevery referenced this issue Dec 27, 2017

Closed

[wip] feat(core): introduce ComponentLifecycle #20682

3 of 3 tasks complete

@ngbot ngbot bot added this to the Backlog milestone Jan 23, 2018

@e-cloud

This comment has been minimized.

Copy link

e-cloud commented Jul 22, 2018

@mhevery any progress on the design? It's has been such a while without further update.

@lazarljubenovic

This comment has been minimized.

Copy link

lazarljubenovic commented Jul 22, 2018

My guess is that the answer is: "it will all be much simpler once Ivy is stable".

@mhevery

This comment has been minimized.

Copy link
Member Author

mhevery commented Jul 25, 2018

My guess is that the answer is: "it will all be much simpler once Ivy is stable".

Correct!

@chaosmonster

This comment has been minimized.

Copy link

chaosmonster commented Feb 13, 2019

Now that Ivy is almost around the corner. Is there any thoughts on this issue?

@mhevery

This comment has been minimized.

Copy link
Member Author

mhevery commented Feb 13, 2019

We will not work on this until ivy is fully landed and default render. Preview is in v8 and default is in v9 https://blog.angular.io/a-plan-for-version-8-0-and-ivy-b3318dfc19f7 so not before than.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.