refactor(bootstrap): Remove support for old bootstrap mechanisms #8147
Conversation
|
Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.
If you need to make changes to your pull request, you can update the commit with Thanks again for your help! |
| forEach(element.querySelectorAll('.' + name + '\\:'), append); | ||
| forEach(element.querySelectorAll('[' + name + ']'), append); | ||
| } | ||
| forEach(element.querySelectorAll('[' + name.replace(':', '\\:') + ']'), append); |
There was a problem hiding this comment.
now that things are much simpler how about we just use querySelector instead and create a single query containing all possible attributes. That way we'll find the first appElement in one query. We can then iterate over the list of names to get the module name.
|
nice cleanup! I suggested more refactorings... |
|
Please also mention in the commit message that class based bootstrap no longer works. |
|
how much does it really gain us to remove these, from app startup time? It's not very much code being thrown away, so it's not clear to me that it's necessarily worth breaking legacy apps even further by doing this. don't take this as a strong objection or anything, I don't care all that much, but at this point it's like breaking things for no real reason --- what do we get out of it? |
| }); | ||
| elements.push.apply(elements, element.querySelectorAll(query)); | ||
|
|
||
| forEach(elements, function(element) { |
There was a problem hiding this comment.
how about we get rid of this and the other nested forEach and move the module name lookup logic into the forEach above?
That would mean that instead of building up the query, we'll actually use querySelector to find the first matching element (we don't need querySelectorAll since we bootstrap only the first one). This way everything is much simple and more efficient for the ng-app use case and we still support the other attribute names.
We should also reorder the the names array to list ng-app followed by data-ng-app as the first two names.
There was a problem hiding this comment.
@IgorMinar will update the PR, note that we will still need 2 forEach (even when these will not be nested) as we need to give priority to element over what we find using querySelector. This is, if element has the attribute data-ng-app and within the document there is another element with the attribute ng-app then we need to bootstrap element
|
@caitp with the other suggestions I made in #8147 (comment) we are making the code smaller, faster and easier to understand, while dropping support for obscure use-cases that as far as I know nobody uses. If someone really cares about these use-cases (e.g. the class based bootstrap), I'm sure that we'll hear about it. You know better than others that I'm very conservative about breaking changes, but if we can make our code significantly cleaner by dropping support for features that nobody uses (or nobody even knows about) then we should do it. |
|
@lgalfaso also this commit should be categorized as refactor and not chore. |
Remove support for bootstrap detection using: * The element id * The element class. E.g. ``` <div id="ng-app">...</div> <div class="ng-app: module">...</div> ``` Removes reference to how to bootstrap using IE7 BREAKING CHANGE: If using any of the mechanisms specified above, then migrate by specifying the attribute `ng-app` to the root element. E.g. ``` <div ng-app="module">...</div> ```
lgalfaso
changed the title
btford
changed the title
Remove support for bootstrap detection using: * The element id * The element class. E.g. ``` <div id="ng-app">...</div> <div class="ng-app: module">...</div> ``` Removes reference to how to bootstrap using IE7 BREAKING CHANGE: If using any of the mechanisms specified above, then migrate by specifying the attribute `ng-app` to the root element. E.g. ``` <div ng-app="module">...</div> ``` Closes angular#8147
Remove support for bootstrap detection using the element id or the element class. E.g.
Removes reference to bootstrap using IE7
BREAKING CHANGE:
If using any of the mechanisms specified above, then migrate by
specifying the attribute
ng-appto the root element. E.g.