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

default Pipes not on by default #3173

Closed
jhades opened this issue Jul 21, 2015 · 6 comments
Closed

default Pipes not on by default #3173

jhades opened this issue Jul 21, 2015 · 6 comments

Comments

@jhades
Copy link

jhades commented Jul 21, 2015

In alpha 31, the default pipes are not on by default. In an application bootstrapped without any extra configuration, the following code:

{{'Hello World!' | uppercase}}

gets rendered as Hello World! instead of HELLO WORLD!. This is in a typescript setup, the problem can be reproduced like this:

  • clone the angular2-seed
  • npm install / gulp serve.dev
  • add the code above on home.html - the string does not get uppercased

Apparently the default pipes should be (as indicated here):

export const defaultPipes: Pipes = CONST_EXPR(new Pipes({
  "iterableDiff": iterableDiff,
  "keyValDiff": keyValDiff,
  "async": async,
  "uppercase": uppercase,
  "lowercase": lowercase,
  "json": json,
  "limitTo": limitTo,
  "number": decimal,
  "percent": percent,
  "currency": currency,
  "date": date
}));
@jessegood
Copy link

@jhades: I noticed that the uppercase and lowercase pipes work by adding
changeDetection:ON_PUSH in your @Component.
However, it should still work with the default I believe.

PatrickJS added a commit to PatrickJS/angular that referenced this issue Jul 22, 2015
same problem as `json` previously of transforming only on reference
check
closes angular#3173
@PatrickJS
Copy link
Member

@jhades @jessegood this was my fault sorry about that. I pushed up a PR with the correct fix #3189. The problem was that I was doing more work than I should by only transforming on reference change. This is why changeDetection: ON_PUSH would allow it to work since you're providing a new reference to the bindings which would correctly transform the value

@gkalpak
Copy link
Member

gkalpak commented Aug 3, 2015

FWIW, I don't think the issue was checking against the last input value, but the fact that in case of a match you would return the input value (not the transformed output).
Since these transforms are very cheap anyway, it's not worth having such an optimization in place anyway.

@PatrickJS
Copy link
Member

The problem was that I used ObservablePipe as code reference since it was the only real pipe at the time (and of course we have no real docs at the time). Then there was also an API changed to remove having to return NO_CHANGE reference when the transformation didn't need to happen (now changed to use WrappedValue). I was also doing smoke tests for these pipes in an app that optimized the change detection for reference checks (changeDetection: ON_PUSH) and the old pipe API.

change_detection/pipes/observable_pipe.ts
ObservablePipe has to maintain reference of the Observable and has essentially 4 states in which the transform does something different that causes side effects: Observable without a subscription, Observable with a subscription, current value/returned value are the same, and current value/returned value are different. I only know this because I had to create an ObservablePipe that prevented a cycle from happening when a change returns a new Observable reference. So there's actually a potential bug where the first state should return the returned value and the returned value should be dereferenced during the onDestroy Pipe lifecycle rather than the Observable reference lifecycle. With all that said I should have went back to refactor the pipes so that's my bad

@gkalpak
Copy link
Member

gkalpak commented Aug 3, 2015

Looking at the commit that closed this issue (4dc6d74), I thought it was the fact that the untransformed value was returned after the initial invokation.

But I don't know much about pipes, so I probably misread that 😞
Sorry for the noise 😈

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants