This repository has been archived by the owner on Apr 12, 2024. It is now read-only.
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
docs($compile): deprecate
replace
directives
BREAKING CHANGE: The `replace` flag for defining directives that replace the element that they are on will be removed in the next major angular version. This feature has difficult semantics (e.g. how attributes are merged) and leads to more problems compared to what it solves. Also, with WebComponents it is normal to have custom elements in the DOM.
- Loading branch information
eec6394
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.
Without replace what is the idiomatic way to have a directive that can hide/show (or ng-if) itself? without needing to write ng-if along side every directive.
specifically I have a directive that creates a "message zone" which is only shown when there are messages to show. Before this change I've simply made a replace directive with ng-if on the main tag.
After this change, unless I add ng-if to the tag containing the directive, the containing element will always exist in the dom.
eec6394
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.
this creates a lot of issues for flex box layout, it also makes the user jump through hoops to modify reusable components via classes or attrs, i would strongly urge that you keep this awesome feature
eec6394
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 am not a fan of this deprecation, to say the least. Replace true is an elegant solution that I've come to love and rely on in many ways. I understand there are challenges, but removing this feature would really suck. It makes my code, styling, and DOM so much cleaner.
eec6394
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.
Hello, i pointed to this place accidently, but it is very important for me.
Now i use replace property of my directives for SEO purposes.
For example i have page with price of some products in table. I want it to be crawled by google and in the same time i want to provide rich interface for user.
So i render that table in my server this way:
And i have myPriceTable directive defined with
replace: true
. This angular directive loads data with$http.get
from my server and becomes a table, user want to see and interact with. Everybody is happy. (seo and user)But what i suppose to do, when replace: true will not be there?
eec6394
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.
@outluch
replace: true
isn't being removed. You can be pretty certain of that, like as cute as it is for us to say it's deprecated, it's not going anywhereeec6394
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 FYI...
eec6394
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.
Example. Now I use
which replace on
I can't include something into input.
But, ok. I use
and I get
and I need copy
input-group
class to element in compile fn (I need devide template and set some classes separetely).Ok. I can include whole template into directive
and I get broken markup
(I had problem with bootstrap framework).
What can I do?
eec6394
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.
@tamtakoe Yes, in your case where you try to wrap elements into others, you can use another element and a directive with a template.
I don't understand why you get broken markup. Could you create a plunker / jsfiddle for this?
eec6394
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.
Example: http://plnkr.co/edit/II3ytLSayP97jNPZlIlc?p=preview
I can't use
input-group
class onmy-password
tag (only oninput
), because it is bootstrap markup. And I need apply styles to immediate children (especially if I work with positions)eec6394
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.
"This feature has difficult semantics (e.g. how attributes are merged) and leads to more problems compared to what it solves."
Can this remark be expanded upon? what are the problematic semantics in how attributes are merged, and what problems does it cause? have there been real cases where such problems arose?
eec6394
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.
@Illniyar one scenario that comes to mind is merging
into
We end up with
Which obviously throws an error because
{active: true}{hasHeader: true}
isn't a valid expression.eec6394
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.
none of that really matters though, because it's never worked in the past, so it doesn't matter that it doesn't work. there are very limited cases where merging can really fail (or rather, can cause "failure")
eec6394
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.
Regarding the ng-class example, why can't angular internally special case the merging of the ng-class attributes?
eec6394
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.
@aliatsis because it's complicated... first of all, the two ng-class expressions might care about different scopes, and it's just not super simple to combine JSON objects in string form, it would add a lot of complexity
eec6394
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 understand. Maybe then, for example, make the ng-class that is in the replacing template change names pre-compile (but still point to the same directive) and bind it to the proper scope. This way both can exist without name conflict or scope confusion. So merging
<my-drtv ng-class="{key1: val1}"></my-drtv>
and<div ng-class="{key2: val2}"></div>
would result in:<div ng-class="{key1: val1}" ng-class-internal="{key2: val2}"></div>
. Just a thought.eec6394
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.
Yeah, maybe something like that would be possible, but the trouble is that theoretically this could apply to literally any attribute, we don't really know which ones would break and which wouldn't. ng-class would definitely, but it could also be ng-init, or some custom directive from some library, like we just don't know.
Unfortunately it ends up not being trivial. But I think it's not the end of the world that we have problems merging these attributes, like it doesn't really matter, for the most part.
eec6394
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.
This comment has two parts to it - first on why I think
replace
should be kept, the second thoughts on a possible solution.Why should
replace
be kept?replace
directive.Last few years, a lot of my angular work was refactoring legacy code to use Angular. Personally much of that work involves iteratively moving pieces of code from legacy to angular one piece at a time.
I've used
replace
a lot of times to make sure that the code I moved from legacy retains the same dom structure, which is really necessary when you have legacy css code that relies on tag names and you don't want to duplicate the css or make overarching changes.I think that moving legacy code to angular is a process that should be aided as much as possible
replace
you can have certain attributes on the element itself (I.E. if I want allnotification
elements to havered
class for instance) - this isn't as straight with using element as tag names (you can do so in the directive's compile phase, but it becomes imperative instead of declarative).This can really help reduce code duplication.
Whether or not this is something that will be used often is the question, but personally I've used this a few times when making directives to be used solely by a certain part of the application.
Possible solution:
I'm wondering why not pass the attribute merge resolution problems to the directive's writer? what if
replace
can be a function that receives both elements and merges them?There are bound to be a very small amount of possible attribute merging when you create a
replace
directive, and it shouldn't be to hard for the directive creator to manage merging attributes, and he should be aware of all the attributes he is using.We can have default merging rules for some attributes (class, ng-class, ng-init).
We can throw an error when a merge conflict arises (similar to how an error is thrown if there is more then one root element in the template).
If making sure directive owners handle duplicates, we can even have replace be an object with properties matching attribute names and each having a function to determine how they should be merged, and then forcing in compile time that all attributes on the root element have corresponding merge functions.
eec6394
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:
That is true and annoying.
However I don't think it's much different then the current issues that directive creators and users need to deal with (priorities in directives, scoping rules - only one isolate scope, declaring a variables in a nested scope will shadow the parent attribute, etc...) .
Also in this case the scoping problems are entirely inside the directive, users of the directive don't have their scoping rules affected (their ng-class will have the same scope before and after a replace, no matter what scoping rule the directive uses), the only things that are affected are attributes on the root element (and only when using new/isolate scope) which are in the control of the directive's writer.
It's just one more thing that a directive creator needs to take into account regarding scoping - much like the fact that new scopes will shadow parent scope, or shared state with using the parent scope directly.
eec6394
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.
Regarding @Illniyar original comment, I haven't seen an answer to the idiomatic way to deal with a custom element directive ('restrict: E') without replace.
As far as I can tell, removing replace essentially means every root element of a directive will need to be a valid HTML tag, with an attribute directive ('restrict: A').
Or am I missing a trick, somewhere?
eec6394
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.
@cognivator you can simply leave the custom element in the DOM--there's no real specific reason to remove it, every major browser treats it as
HTMLUnknownElement
(and in the future, with Web Components, you can simply define the element and then it exists). Sure, validators won't be happy (currently), but frankly I haven't used a validator since XHTML stopped being a thing. Legacy codebases will, obviously, require careful support, which may mean attribute directives only.eec6394
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'd argue the opposite direction as many comments on this thread. Leaving the original directive in the DOM helps developer debugging, otherwise its much harder to understand what has happened.
If you don't like seeing custom node types when you inspect the page, then use attributes - and leave the directive wrappers in place.
eec6394
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.
eec6394
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.
@lord2800 This isn't the case when your custom element is part of an SVG tree. Browsers won't display any SVG elements under the unknown tag.
eec6394
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.
Ah, SVG... my eternal nemesis. Point taken, @dlmanning.
eec6394
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.
@lord2800
Seems that IE11 makes difference between a piece of DOM mappend inside an unknown element. Try the following in Chrome and IE and see the difference by yourself:
IE11 custom element in table bug
The only solution I've found so far is to use the
replace
flag, so that flag should not be deprecated.eec6394
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.
The way I see it, the removal of this feature will lead to less declarative code or a lot more repetition.
A thing that I find sometimes useful is to decrease code repetition by encapsulating attributes in a directive. The alternatives without replace are to either:
a) repeat the attributes every time we declare that directive (or not having it all and just repeating code)
b) add those attributes via JS on the linking function
c) put up with an extra useless element
Not to mention that extra elements might cause some woes with CSS and styling, and, on very complex apps and layouts, add some performance overhead. I mean sure, it probably won't be an issue every time and you can get around it for those cases where it's really necessary, but feels pointless and unnecessary.
This just generally feels like what we have seen for a long time in regards to W3C specs: making things easier for the life of the implementors instead of the making it easier for the actual users of the technology.
eec6394
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.
@trodrigues
The opposite in fact. With
replace: false
you clearly see in the template which directive was used to render this part; it'sreplace: true
that is imperative in that it requires element swapping on a very low level, causing numerous problems.I'd go for
(b)
if you need it.Generally speaking, the best approach is to use two kinds of directives:
<custom-element custom-attribute></custom-element>
that create a new structure, potentially transcluding the content.In this way it's clear just by looking at markup what kind of directive you're looking at.
I don't believe you can get any meaningful performance overhead from just having one additional tag. On the contrary; since
replace: true
directives require swapping a living template, if you need to retain data attached to the element, you need to copy it manually (current code uses some weird private jQuery interface which breaks with newer jQuery versions so it'll go away).I often see this claim that "implementor convenience" is sth completely separate from what problems user experience. I disagree with it. The fact that
replace: true
directive cause so many problems result in many edge cases that simply don't exist without this mechanism; these edge cases later go to directive API consumers. The harder sth is to implement, the more hacks we need to employ to just make it work, the more bugs are propagated to developers. Just look at:https://github.com/angular/angular.js/search?q=%22replace%3A+true%22&ref=cmdform&type=Issues
The bugs are there even if you personally haven't experienced them.
eec6394
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.
So now I'd have to manually add a
data-ng-class
to my main directive element, not sure how that'll work.eec6394
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 also (-1) on this. There are many libraries that rely on the fact that some elements are immediate children of another element. This will break directives in code that uses these libraries. One such library is Bootstrap, but there are many others.
I think that a not-perfect solution is better than no solution in this case.
eec6394
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 believe replace: true breaks stacked directives if the top-level element in the directive has an ng-if=, see Issue #8748
eec6394
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.
This is just looking forward to Angular 2.0. Isn't the authoring of directives so completely different in 2.0 that marking it as deprecated here sort of useless?
eec6394
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 new to angular, so I don't have a strong opinion about this feature yet. However, I want to callout a use case where this feature would be very useful for me. This stackoverflow question ( http://stackoverflow.com/questions/26186007/remove-directive-from-the-dom) describes my use case.
eec6394
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.
-1
eec6394
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.
@goooooouwa sorry dude :( but good news, replace directives aren't going anywhere, so feel free to keep using them. Just be wary of the known bugs that can't be fixed
eec6394
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.
Thanks @caitp. good to know that. :)
eec6394
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.
@gimmi if IE11 breaks than this shouldn't be removed as Win7, Win8 and WP8.1 will then break when this doesn't work. Then this simply cannot be deprecated as of now.
Wouldn't another solution be to separate this in a new module/extension to allow people who need this to include it in their project (much like angular-route and angular-sanitize are separate)? I'm not sure if this is possible since directives are pretty much a basic construct of angular, but perhaps we need people to decide for themselves to see if they need this or not.
I've also used replace in the past. Not because i really needed it, but because it lets me see how everything ended up. I'm also not sure if the performance is different with or without replace (perhaps a benchmark can give a solution?), but it seems that this is a trivial change that might require some more thought and research? Since this post is referenced in the 1.3.0 changelog, i recon more people will come here to complain.
eec6394
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.
Hi @Martinspire , see @caitp 's response above, it seems replace isn't going to be removed, but not encouraged to use.
eec6394
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.
Regarding the IE11 issue, the problem will disappear once you declare a display property on that custom element (for example, display: block), which seems should be done anyway, for any new elements introduced into DOM.
Here's a screenshot:
http://www.browserstack.com/screenshots/081f51a53811bf8795f177e9397b24ef8687db6f
eec6394
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 fail to understand how directives that generate SVG markup would work without
replace:true
. The<svg>
tag must have<path>
(etc) tags directly within it, not in some other wrapper tag. Both @lord2800 and @dlmanning discussed this briefly above, but I think this is a very important use case and needs to be called out.For instance, consider this Piechart directive:
https://github.com/crudbetter/angular-charts/blob/master/src/piechart.js#L80
Usage:
Without
replace:true
, the SVG will break because of nestedpiechart-slice
tags:With
replace:true
, it will work as expected:eec6394
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.
The only issue is the scope that I can see. No need to merge anything
Simplest solution...
when using 'replace: true', the directive transforms into...
eec6394
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.
What is the status of this today? I'm using Angular 1.6.1 and the docs say that it's going to be deprecated:
https://code.angularjs.org/1.6.1/docs/api/ng/service/$compile#replace
eec6394
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.
@digeomel: The dcs say that it is deprecated (it has been for almost 3 years now) and that is will not be available in the next major version (2.x). There are no plans on removing it on 1.x, because doing so would do more harm than good. That said, we don't dedicate resources to fixing
replace
-specific bugs and strongly recommend against using it in new projects.eec6394
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.
Thanks (ευχαριστώ!) for the quick answer @gkalpak , but I think everyone would appreciate a suggested workaround to this. I've done some quick search and it seems that people suggest not replacing at all, as the custom tags are not recognised by the browsers and don't do any harm. But surely there must be another way to achieve the same result as with replace?
eec6394
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.
@digeomel We only deprecated it instead of removing precisely because it's not always a quick & easy fix in existing apps using
replace
. There is no equivalent functionality; the problems withreplace
that led to its deprecation are exactly the problems with not using the wrapper element. It's advised to not strip out the component tags in new code.eec6394
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.
bullshit!