Skip to content
This repository has been archived by the owner on Apr 13, 2022. It is now read-only.

Commit

Permalink
Browse files Browse the repository at this point in the history
docs(CHANGELOG.md): add upcoming breaking change
  • Loading branch information
jeffbcross committed Sep 9, 2014
1 parent d1318f7 commit fb39e32
Showing 1 changed file with 18 additions and 0 deletions.
18 changes: 18 additions & 0 deletions CHANGELOG.md
@@ -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.

Copy link
@gkalpak

gkalpak Sep 21, 2014

Member

I just noticed the "and function call expressions" part. What is this supposed to mean ?

This comment has been minimized.

Copy link
@caitp

caitp Sep 21, 2014

Contributor

there were two versions of what we landed, one affected just filters, the other affected everything (only the filter thing landed though, iirc)

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 21, 2014

Member

Lucky us !
Having to make all function call expressions "pure" or having to pass every required non-primitive object as a parameter would be hell (not to mention ruin the team's reputation as "closure fans" :)).

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.

Copy link
@petebacondarwin

petebacondarwin Sep 21, 2014

Member

... would break if it depends on data ...

<span ng-bind="cost|i18nLocalizer">
```


<a name="1.3.0-rc.1"></a>
# 1.3.0-rc.1 backyard-atomicity (2014-09-09)

Expand Down

36 comments on commit fb39e32

@realityking
Copy link
Contributor

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.

@petebacondarwin
Copy link
Member

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.

@rubenv
Copy link

@rubenv rubenv commented on fb39e32 Sep 10, 2014

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?

@bvaughn
Copy link

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

@rubenv
Copy link

@rubenv rubenv commented on fb39e32 Sep 10, 2014

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.

@ChadMoran
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bump the major version?

http://semver.org/

@caitp
Copy link
Contributor

@caitp caitp commented on fb39e32 Sep 10, 2014

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

@ChadMoran
Copy link

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

@bvaughn
Copy link

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" />

@caitp
Copy link
Contributor

@caitp caitp commented on fb39e32 Sep 10, 2014

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

@ChadMoran
Copy link

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.

@caitp
Copy link
Contributor

@caitp caitp commented on fb39e32 Sep 10, 2014

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" :>

@findar
Copy link

@findar findar commented on fb39e32 Sep 10, 2014

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

@rubenv
Copy link

@rubenv rubenv commented on fb39e32 Sep 10, 2014

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.

@rubenv
Copy link

@rubenv rubenv commented on fb39e32 Sep 10, 2014

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.

@jeffbcross
Copy link
Contributor Author

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

@bvaughn
Copy link

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

@Narretz
Copy link
Contributor

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.

@otac0n
Copy link

@otac0n otac0n commented on fb39e32 Sep 10, 2014

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.

@Narretz
Copy link
Contributor

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

@caitp
Copy link
Contributor

@caitp caitp commented on fb39e32 Sep 10, 2014

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.

@ChadMoran
Copy link

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?

@otac0n
Copy link

@otac0n otac0n commented on fb39e32 Sep 10, 2014

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.

👎👎👎

@caitp
Copy link
Contributor

@caitp caitp commented on fb39e32 Sep 10, 2014

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.

@otac0n
Copy link

@otac0n otac0n commented on fb39e32 Sep 10, 2014

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.

@ChadMoran
Copy link

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.

@caitp
Copy link
Contributor

@caitp caitp commented on fb39e32 Sep 10, 2014

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

@ChadMoran
Copy link

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.

@caitp
Copy link
Contributor

@caitp caitp commented on fb39e32 Sep 10, 2014

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.

@petebacondarwin
Copy link
Member

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.

@OverZealous
Copy link

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.

@Siecje
Copy link

@Siecje Siecje commented on fb39e32 Sep 11, 2014

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?

@caitp
Copy link
Contributor

@caitp caitp commented on fb39e32 Sep 11, 2014

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 /).(\)

@petebacondarwin
Copy link
Member

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.

@vitaly-t
Copy link

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.

@Narretz
Copy link
Contributor

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.

Please sign in to comment.