-
Notifications
You must be signed in to change notification settings - Fork 27.5k
Commit
- Loading branch information
There are no files selected for viewing
Original file line number | Original file line | Diff line number | Diff line change |
---|---|---|---|
@@ -1,3 +1,21 @@ | |||
# NOTICE: Pending Breaking Change | |||
|
|||
The next 1.3.0 release candidate (1.3.0-rc.2) will contain a perf-related change that is likely to | |||
introduce breakages in some applications. The change will affect filters and function call | |||
expressions, and will not call the function if the variables passed to the function are primitive | |||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong.
caitp
Contributor
|
|||
values and have not changed since the last digest loop. | |||
|
|||
Example: | |||
|
|||
```html | |||
//date filter would only be called if the 'timeCreated' property has changed | |||
<span ng-bind="timeCreated|date"></span> | |||
|
|||
//custom filter would break if depends on data changed by user other than 'cost' | |||
This comment has been minimized.
Sorry, something went wrong. |
|||
<span ng-bind="cost|i18nLocalizer"> | |||
``` | |||
|
|||
|
|||
<a name="1.3.0-rc.1"></a> | <a name="1.3.0-rc.1"></a> | ||
# 1.3.0-rc.1 backyard-atomicity (2014-09-09) | # 1.3.0-rc.1 backyard-atomicity (2014-09-09) | ||
|
|
||
|
36 comments
on commit fb39e32
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.
Will there be a workaround for this? There are a couple use cases I'm not sure how to implement otherwise. Besides internationalization related filters, there are also filters responding to events or timeouts or ones relying on async responses.
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.
@realityking - there are a number of workarounds that should be documented in the changelog and also in the docs. The best solution is to make all your filters "pure", i.e. to have no side-effects and not rely on anything other than their input parameters. In the case of localization it was decided that changing localization mid-app was a very unusual case - almost always changing locale would involve a refresh of the page, in which case the filter cache would be recreated.
If you have specific use cases where this is difficult to workaround please do let us know as soon as possible in case changes to the refactoring are needed.
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.
Related: rubenv/angular-gettext#106
This is quite problematic for us. One option would be a call to clear the filter cache, which we could trigger when the language changes. That would allow a one-time override of the caching behavior.
Would that be something the Angular.JS could consider?
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.
@realityking: This might not be the kind of workaround you had in mind, but...perhaps consider switching away from something as expensive as a filter function to a directive for localization? Your directive could then listen to $rootScope
for locale-change events and update its content accordingly.
Something like...
angular.module('myApp').directive('i18n',
function($rootScope) {
var getLocalizedValue = function($element, $attributes) {
var key = $attributes['i18n'];
var data = $attributes['i18nData']; // In case your string requires interpolation
var locale = $rootScope.locale; // Or wherever your app stores it
var localized = ''; // Perform your localization here
$element.text(localized);
};
return {
restrict: 'A',
link: function($scope, $element, $attributes) {
$rootScope.$on('localeChange', function() {
updateLocalizedText($element, $attributes);
});
$attributes.$observe('i18n', function() {
updateLocalizedText($element, $attributes);
});
$attributes.$observe('i18nData', function() {
updateLocalizedText($element, $attributes);
});
}
};
});
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.
@bvaughn: Filters have a valid use case: attributes. Here's an example: http://angular-gettext.rocketeer.be/dev-guide/annotate/#attributes
Directives alone doesn't cover all situations.
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.
Bump the major version?
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.
these breaking changes are peanuts compared to the ones that may one day ship in v2, should such a day ever comer. Just treat the minor revision as a major revision for now =)
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.
There is another number after 2. Making a breaking change and not bumping the major version is most likely going to create a lot of pain for users. Not only that if you're treating the minor version as a major version now minor version changes need to increment the patch version...
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.
@rubenv Sure. I didn't say that directives cover "all situations". I only said that a work-around for some types of situations (like the locale example mentioned) could be achieved using an attribute directive.
But... I could update my directive example to cover the example you provided above by exposing a 3rd optional parameter to my directive called "i18n-attribute" that- if present- informed the directive that its localized string should be written to an attribute instead of the inner text.
var getLocalizedValue = function($element, $attributes) {
var key = $attributes['i18n'];
var data = $attributes['i18nData']; // In case your string requires interpolation
var attribute = $attributes['i18nAttribute']
var locale = $rootScope.locale; // Or wherever your app stores it
var localized = ''; // Perform your localization here
if (attribute) {
$element.attr(attribute, localized);
} else {
$element.text(localized);
}
};
Then you could use it like so:
<!-- Localized text goes inside of the button -->
<button i18n="myButtonKey"></button>
<!-- Localized text goes inside of the "placeholder" attribute -->
<input type="text" i18n="placeholderLocaleKey" i18n-attribute="placeholder" />
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.
if we bumped the major version every time there was a breaking change, we'd be up to angular 9000 by now
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.
Then I guess its a good thing that package managers generally default to auto updating anything under the same major version.
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.
Don't use "^1.2.0" :>
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.
Would this still update dynamically as expected when someLocale is updated?
someString | translate:someLocale
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.
@bvaughn That still wouldn't work if you would have multiple attributes. Writing a directive per attribute is also madness. Filters exist for a reason, we wouldn't have them if everything was possible with a directive.
@findar That would work, but that's quite insane, right? It'd add a lot of noise that shouldn't be there in the first place.
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.
@jeffbcross Is there a way to selectively wipe the filter cache and force a re-evaluation? That would be a best-of-both-worlds situation.
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.
Sorry folks, I intended to add a link to the PR in question to the Changelog. Comments regarding implementation should be added here: #8942
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.
@rubenv Once more- I'm not trying to suggest that filters are never useful or appropriate. Just that in many cases there are acceptable work-arounds. In this specific instance, I believe I could iterate on the localization example to cover your concerns- but since it's just an arbitrary example it probably wouldn't be worthwhile. I only mentioned it in case it gave ideas to others who would need to rethink their filter strategy.
I happen to like this change in terms of the performance gains it offers. :)
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.
@petebacondarwin Dismissing i18n modules that change the local at runtime as "unusual" is really strange. Angular supported this use case, and I can only see this becoming more common rather than less.
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.
Considering that Angular is published in bower, and bower uses semver...
Also considering that every other package manager either explicitly or accidentally uses semver...
Also considering that, if you want your version numbers to mean anything, they need to have some definition...
You should bump the major version number for breaking changes like this.
👎 to leaving this as v1.
Dear lord, who cares if your version number get big? I'm using chrome version 36.0.1985.143
right now. So now, who cares if 36 is a "big" number? Nobody, that's who.
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.
There's another PR with a different implementation here: #9006 which uses a flag for filters that rely on externalInput. This is probably worse for overall performance than the ability to clear the filter cache
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.
@otac0n, bower uses semver, bower doesn't require you to ask for "^"-behaviour. you can ask for a specific version, and upgrade when you're ready. It's not that bad. Angular being packaged on bower is not really a driver for the versioning model --- all of these breaking changes are coming in before 1.3 goes stable, at which point there will most likely (read: it would be a very bad idea) to include any subsequent breaking changes in the 1.3 lifecycle. If you're using 1.3.0, you should not see breaking changes. (You aren't using 1.3.0 yet because it doesn't exist yet).
Anyways, for what it's worth, I did defend this point of view, that locale-dependent filters in particular being broken would make people unhappy, but we decided that the performance impact was more important. We want to make the framework powerful, but of course we also want to minimize the cost. Filters are already pure in AngularDart, and we generally agree that treating filters as pure is the right thing to do. Unfortunately, this doesn't fit well with existing modules in the ecosystem which allow changing $locale, but we do see some really nice perf improvements, so overall this is a good thing.
I think an option to clear the filter cache might not be a terrible idea, too, in order to help work around it.
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.
http://blog.angularjs.org/2013/12/angularjs-13-new-release-approaches.html
I don't think anyone is questioning the validity of the change. Just the version.
So the idea is to start using semver in 1.3? For the first version of your semver usage you're going to introduce a breaking change?
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.
@caitp, if Angular is in a package manager that requires semantic versioning, then the package version must change according to semver. What will have to happen is Angular 1.3 will be packaged in bower (and all other package managers) as 2.0
leading to much confusion.
This happened to log4net and NuGet, and will probably happen here.
From the article:
Just to be clear, the log4net 2.0 NuGet package contains the log4net 1.2.11 assembly.
Considering the official blog post from almost a year ago that @ChadMoran mentioned suggests that Angular will be moving on to semver in the future, I don't see what benefit there is in calling this version 1.3
.
👎👎👎
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.
I'm sorry you're unhappy with it :( Bower will only automatically update the latest in the major release if you ask it to, so don't ask it to in the case of angular.
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.
Perhaps if you would, you know, give the reasons behind not changing it there would be less unrest.
As of yet, the only reason given has been "9000 is big lol," which is bogus since Angular hasn't even seen 100 non-beta releases yet. (I count 66 tops, few of which are breaking changes.) Actually, it's doubly bogus because past versioning habits don't matter.
What does matter is that an official blog post said that Angular would be moving toward semver.
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.
@caitp I think we understand you can change the version control in package managers but people shouldn't have to know that AngularJS might introduce a breaking change so now I have to lock my version down to the patch version number.
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.
Guys, the rules are outlined here, just follow them, that's it.
This is not a productive discussion to be participating in, so lets can it =)
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.
That's a very dogmatic approach to a concern by the community. This discussion isn't productive because you haven't validly defended the decision.
Obviously you've made up your mind about this, though. Have fun breaking projects.
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.
I'm not the one making these decisions --- I don't think this version number matters in practice, it is essentially just a number, and semver-based package managers provide numerous ways to deal with this (including npm and bower). Many projects are not using a package manager at all, many are using CDNs --- the version numbering strategy is irrelevant to most of these, and irrelevant to semver too if you follow the rules.
I did not make the decision to call it 1.3, but I don't think it's likely to change, and it's somewhat inappropriate to complain about this on this issue which is unrelated to this. If there is a concern with the versioning strategy, I suggest bringing it up on angular-dev or filing a bug regarding that in isolation from this commit.
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.
@ChadMoran et al - I think it is fair to come clean and say that we are indeed not following semver for Angular 1.3 :-| It is kind of a transitional... Your concerns are valid if you come from a belief that we are strictly following semver.
The versions up to 1.2 followed the completely different system of odd minor numbers corresponding to "unstable" versions and even numbers being so called "stable".
From Angular V2 onwards we have a very different situation - V2 is going to be more of a marketing term (like "new" Angular) - but Angular V2 will actually be constructed from a number of independent modules which will indeed follow semver individually while living under the banner of V2.
For 1.3, we are going to make breaking changes from 1.2 but from then on 1.3.x will not contain breaking changes.
In any case if your project uses AngularJS via "1.2.x", or "~1.2.n", where n is some number, then it will not automatically update to 1.3. So there should be no unexpected surprises there. If you use "^1.2.n" then it will indeed update automatically.
As @caitp said this is what has been decided and since after 1.3 the majority of the development work will be focussed on modules under the V2 banner then this should go away before the next major release. Hopefully :-) Please bear with us in this transition period.
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.
Would it make sense for a filter to have an opt-out defined on the filter function. This is just me brainstorming, but what about allowing a property to be defined on the function object, something like this:
module.filter('myfilter', function() {
function myFilter(value) {
//...
}
myFilter.$useFilterCache = false;
return myFilter;
});
Then if a filter function has $useFilterCache
defined and ===
to false
, it is never cached. (Alternatively, invert the name and change it to something easier to check, like $noFilterCache = true
.)
Oops I meant to post this on the PR, ignore it here.
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.
A better question is why are you adding breaking changes in a release candidate?
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.
@Siecje that is a valid question, and one which has been discussed by the team --- the goal as stated, if I recall, was to get all of the remaining breaking changes into rc1, and not add any additional ones. (No promises, anything is possible /).(\
)
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.
@IgorMinar has a view on this.
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 a fight for common sense, it is simply not acceptable to introduce breaking changes within an RC version. If it didn't happen prior to the first RC, then there is no room left for any breaking change any more. If you try to look at it in any other way, you are laying a dangerous road in front of you, it is not just a technical precedent, it is guaranteed to upset lots of developers.
My team has delivered a few successful products with 1.2.x without ever running into any shortages supposedly resolved in 1.3.x, and with you breaking 1.3 like that it is a serious turn-away from an early upgrade.
Please get back to earth and do what's right.
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.
angular introduced breaking changes in rc.3 of 1.2, and also in the 1.2.0 release (!). I am not saying this is a good thing, but it has happened before and the consequences were not catastrophic.
I just noticed the "and function call expressions" part. What is this supposed to mean ?