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

fix($locale): make all filters $locale aware (even after $locale has changed) #9160

Closed
wants to merge 1 commit into from

Conversation

dragosrususv
Copy link

Closes #9159 .
I do not expect for this to pass without unit tests - I will add those as soon as the problem is confirmed as relevant.

@caitp
Copy link
Contributor

caitp commented Sep 18, 2014

See fca6be7 (even if they do reference $locale[FIELD] directly, they will still not know to run themselves again unless they're $stateful --- Igor said this was a good trade off because of the performance and couldn't see the use case for dynamically changing locales, so you probably won't see this land)

@dragosrususv
Copy link
Author

@caitp : You might be right, but then again, why wasn't this applied in all filters? (in particular the date filter)

I doubt the impact will be big here - do you want me to create some performance ops graphs?

As for the use-case itself, it's like cutting the branch of your feet - think of a scenario of a SPA where users can login and logout.
Firstly, you set the locale of the browser or maybe something from localStorage or pouchdb (if the user previously accessed your app).
Secondly, you need to load another $locale when the user logs in - lets say the login is in en_US but the user preference is in nl_NL - so there you go, load Dutch.
Thirdly, the user might as well logout and login again with another account (which has fr_FR as preference) - again, load French.

If the 3rd case might be a corner case one, the 1st and the 2nd will be 90% of the users logging in any SPA with user locale preference.

I don't understand why this isn't a valid case. Or at least: what is the AngularJS suggestion here?

@IgorMinar , @pkozlowski-opensource : does this make sense to you as well?

P.S. @caitp : I highly appreciate your responsiveness but please remove the Purgatory milestone - I truely believe AngularJS is a good framework and it has it's place in future web dev. Lets make it better.

@caitp
Copy link
Contributor

caitp commented Sep 18, 2014

I'm not the one you have to convince --- I did argue that this would probably upset people, however we do see some measurable performance wins in the largetable benchmark, where we get huge improvements just by avoiding [[CALL]] --- but it's actually significantly higher when the filters are doing meaningful work. So we like that we can do things fast --- the $stateful property means that we have to re-evaluate the filter every digest because we can't treat it as pure, and most of the time this is what we want.

We might be able to make some of the core filters stateful (/cc @IgorMinar / @tbosch) and rely on $locale, but the decision so far has been that dynamic locales are a weird use case that you typically wouldn't need

@dragosrususv
Copy link
Author

@caitp I didn't want to offence anyone. I haven't contributed to AngularJS before and I'm sort of expecting of providing "bad input" (code wise, perf wise). Still, the collabs or core devs have far better visibility of the whole architecture, problems and known limitations - so they can come up with a better vision of the solution for the problem.

What I don't understand is why isn't the above a real use case? It's like AngularJS is suggesting the community to fully reload the page after each login - that can't be I presume, just asking.

In any case, thank you kindly for spending your time writing here.
I'm an optimist, so I still hope for a solution here :)

@mary-poppins
Copy link

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@caitp
Copy link
Contributor

caitp commented Sep 18, 2014

Yeah I hear you, I did say that the pure filters thing would make people who depend on things like dynamic locales unhappy, but the general feeling was that making it fast was more important.

It should still be possible to work around these limitations, perhaps by decorating core filters, or some other solution.

I think it would be nice to make everyone happy, but that's very hard to do. Lets wait for Igor or Tobias's input to see if there's anything we can do to help the dynamic locale use case in some way. It may be that a third party module is the only solution that satisfies everyone

@mary-poppins
Copy link

CLA signature verified! Thank you!

Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes).

@dragosrususv
Copy link
Author

Thank you kindly @caitp . Any remarks @IgorMinar / @pkozlowski-opensource / @tbosch ? What would be the proposed AngularJS way for this problem?
To answer upfront in the context of @btford's comment: yes, performance is important for this app. But this feature is again needed.

@caitp
Copy link
Contributor

caitp commented Jan 6, 2015

@dragosrususv I understand that it's frustrating, but I don't think we can undo this at this time.

However, there is some good news: it's relatively simple to make core filters stateful, like so:

http://plnkr.co/edit/qvzXkQkVtTLHvHVYXTdU?p=preview

@caitp
Copy link
Contributor

caitp commented Jan 6, 2015

Given that you can work around this in 4 lines of code (per filter that you care about), I feel like this can be closed... Sorry :|

@caitp caitp closed this Jan 6, 2015
@dragosrususv
Copy link
Author

@caitp : thank you kindly for your example - I just saw this message, don't know how it slipped.
That's a very good plunker, exactly what I was looking for.
Would be nice if we could add this plunker in filters documentation - to mention that they are stateless by default and they may become stateful through your example.

Nice!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not all AngularJS filters are $locale aware after lang+locale changed
3 participants