angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.86k stars 27.52k forks source link

1.2.0rc1 regression: script tags not loaded via ngInclude #3756

Closed kamstrup closed 10 years ago

kamstrup commented 11 years ago

If one loads jQuery before Angular, loading and execution of script tags in the html fragments with ngInclude works under 1.0.* (just tested 1.0.7 here with jq 1.10.2). This is broken in 1.2.0rc1

meznaric commented 11 years ago

Similar question appeared on SO http://stackoverflow.com/questions/18457804/ng-include-stopped-working-after-update-to-1-2-0rc1

kamstrup commented 11 years ago

Tested with snapshot (1.2.0-6b91aa0) from 26-08-2013, which reportedly fixed the issue where ngInclude was broken with ngRepeat; but it doesn't seem to fix this particular issue.

IgorMinar commented 11 years ago

@matsko can you please take a look at this?

this previously worked only by accident because jQuery evaluated scripts found html to be passed to #html() function. this feature never worked with jqLite.

matsko commented 11 years ago

Taking a look in the morning tomorrow.

njs50 commented 10 years ago

problem stems from /src/ng/animate.js : enter() using the insertBefore() method instead of jQuery/jqLite after() or append(). Would be an issue with any other animate.enter() w/embedded script.

src/ng/directive/ngInclude.js line 194 src/ng/animate.js line 99

looks non trivial to fix due to other animation related things depending on the insertBefore() having been used

matsko commented 10 years ago

yeah insertBefore() was a new addition for the animation rewrite. I'll try to see if it can be removed.

njs50 commented 10 years ago

I switched it out to .append() and .after() and it broke j ~160 tests. I think because the element was being mutated by them.

olostan commented 10 years ago

Wow... this is not a good news... some of 3rd party components I'm using in a project built with AngularJS use <script> tag to adjust some properties and I've put that into <script> tags inside of partials....

This is breaking issue for me before switching to 1.2 from 1.1.*.... Is there any ideas how it could be solved? May be import code, that stripped/evaluated <script> tags from jQuery?

btford commented 10 years ago

The following test works in master, as well as 1.2.0-6b91aa0:

window.evaluated = false;
$templateCache.put('tpl', [200, '<script>window.evaluated = true;</script>', {}]);
element = $compile(html(
  '<div><div ' +
    'ng-include="\'tpl\'">' +
  '</div></div>'
))($rootScope);
$rootScope.$digest();
$animate.flushNext('enter');

expect(window.evaluated).toBe(true);

@kamstrup, can you make a fiddle/plunker that shows the issue?

btford commented 10 years ago

The behavior of jQuery has changed since 1.9.0 and they are not supporting this feature anymore.

If you want to preserve this behavior and use a newer version of jQuery, you could decorate ngInclude to eval the contents of scripts after a template is resolved. I would recommend re-evaluating your strategy for asynchronously loading content, and coming up with a better solution.

njs50 commented 10 years ago

jQuery still supports this btw, the only change is that they no longer remove the script tag from the dom after executing it.

jQuery release notes: As of 1.9, scripts inserted into a document are executed, but left in the document and tagged as already executed so they won't be executed again even if they are removed and reinserted.

my workaround was to copy/rename the pre animation version of ng-include. At the moment it's just being used to load a bunch of old (pre angular) content into the views and one day someone will have time to update them all and we can ditch it.

@btford, I tried your test in both master and 1.2.0-6b91aa0 and both failed. Looking at the code it seems $animate.enter() is still using insertBefore(), so no jQuery call at all. Not sure how the tests could of passed given that?

here's a plunker showing the issue : http://plnkr.co/edit/FyJX8Q7qvfWb0BZwLF2g?p=preview

olostan commented 10 years ago

If desired behaviour couldn't be achieved, may be it would be good to put this note into documentation? I assume some developers could expect this behavior as it is in JS (even without removing executed script as it is in 1.9+).

endorama commented 10 years ago

Can someone explicitly tell if this feature will be added again? I do think is pretty common to need jquery ( mainly because a lot of front end framework uses that, bootstrap and foundation for examples ), and needing an old version of jquery or angular to have this feature is a little sad.

I tested this behaviour with jquery 1.10.2 and angular 1.0.8 and is perfectly working...

Thanks for your efforts :)

btford commented 10 years ago

@endorama you could always augment ngInclude to add this feature if you really want it.

endorama commented 10 years ago

Can you explain how to do that? I could do that but I've still some difficulties in advanced angularjs development!

:) Thanks

njs50 commented 10 years ago

Just to be clear, this isn't a jQuery issue.

The issue is that jQuery in no longer being used to insert the content into the DOM (as of animations being added).

If you wanted to work around this you'd probably need to decorate the ngAnimation "enter" function so that any script tags in the content were removed and evaluated before adding the content into the DOM.

for a temporary solution i just copied the old ngInclude.js over from the angular src and renamed it, so I could use it until I have time to update the places where it's required.

On Tue, Oct 15, 2013 at 7:55 AM, Edoardo Tenani notifications@github.comwrote:

Can you explain how to do that? I could do that but I've still some difficulties in advanced angularjs development!

:) Thanks

— Reply to this email directly or view it on GitHubhttps://github.com/angular/angular.js/issues/3756#issuecomment-26279411 .

rolbr commented 10 years ago

I tried copying the ngIncludeDirective from 1.1.5 which I think is what @njs50 suggested, but ran into a lot of funny errors. For now I'll just include my scripts in the main document. However, telling the templates to "bring your own script" was a very nifty feature before 1.2. I'm working on a mobile app that pulls in additional content via ng-include. That content may or may not rely on additional scripts (slideshows etc), but as it currently stands, there's no way for me to know which scripts a certain template might need at some point in the future. I really do hope this feature doesn't get dropped, even more so since @IgorMinar said it only worked by accident for previous versions ;)

neemzy commented 10 years ago

Quick fix : replace the script tag you want to load by a <div load-script></div>.

Then write a loadScript directive that creates your script tag and appends it to your div.

Took me one minute.

endorama commented 10 years ago

Thanks @neemzy, but still I'm having some problems! I tried to look at the code for ngInclude, to use that as a starting point for the load-script directive but I'm not enought expert on angularjs to fully understand that code and build my solution...

Would you be so kind in posting an example, a snippet or anything else that could point me to the right direction?

I thought that a directie like ngIncludeScript could be really handy for this use cases, but seems that I'm not able to create it without help... :)

olostan commented 10 years ago

@endorama Here is a sample of quick-and-dirty workaround: http://plnkr.co/edit/ufCOShc2EaZSzjiOmfOD?p=preview

neemzy commented 10 years ago

@endorama Here's what I did, it's really a simple dirty example but you get the idea : http://www.dpaste.cc/paste/5279208b192d306411000003#javascript

It will fill an element with load-script attribute with your script tag.

endorama commented 10 years ago

I ended up doing as @olostan suggested! Thanks everyone for the help!

I created a gist with a little angular module ( with comments and attribution!! ).

Just one last question: what about secutiry, using eval in this way? As we are loading "our" scripts there should be no issues in using eval, but I'm still a little unsure about this.

BrkCoder commented 10 years ago

is this issue going to be handle soon?

neemzy commented 10 years ago

@BrkCoder as it only ever worked if you were using jQuery, I doubt this is going to be fixed. I think I remember reading so somewhere but can't remember where. You have solutions in the comments here. :)

BrkCoder commented 10 years ago

But your solutions are limited!! I tested it myself,with all the respect. It isn't ok that I need to search for all kind of mini scripts to fix apps because angular.js 1.2.0 break all of their implementation logic. Again your solution is ok but doesn't cover all the cases,it is just a private solution. :/

neemzy commented 10 years ago

@BrkCoder As I said, Angular breaks nothing. It worked because of jQuery but was never intended to be a core feature in the first place.

Of course solutions presented here are not bulletproof, these are just people facing the same issue as you and sharing what they did to get around it. These are examples, guidelines to help you craft your own solution, adapted to your own needs.

BrkCoder commented 10 years ago

I see. Anyway for me it is look like a very main feature, angular.js doesn't give me any alternative tool to operate scripts from other html pages,too bad that they aren't going to take it into their concern.

neemzy commented 10 years ago

I must say I don't really understand why using a very simple directive to inject your script tag is not an acceptable solution to you, but I guess you have your reasons. I won't answer here any further to avoid polluting the thread even more, if you ever want to continue this conversation feel free to leave me a message through the contact form of my blog (the link is on my profile) :)

endorama commented 10 years ago

@BrkCoder You could help in adding use cases into proposed solutions! This feature had "never worked". Was not planned nor was "consciously" introduced. So will never be "fixed", because has never been broken.

If you need this feature as much as everyone else who had commented this issue, please contribute to an angular module to properly implement the feature. That would be the better end for this problem! Or propose your solution for your use case, and someone else could try to arrange a decent module with good use case coverage...

IgorMinar commented 10 years ago

UPDATE

I'm reopening this issue as it keeps on causing issues for several folks. We need to investigate this further.

To sum iup the previous history of this issue: we closed the issue since the whole script execution thing relied on jquery doing extra stuff behind the scenes (parsing, fetching and evaling scripts).

I believe that jquery 2.0 doesn't do this any more, but we should double-check. If it still does this, then we should investigate and see if there is something we can do to make it still work.

The issue might be in that previously we used jquery's html() method to turn string into DOM and this method was executed on an element that was part of the document. This caused jquery to eval the script. With animation changes, we do things slightly differently and it's possible that when we turn string to dom the parent node is detached. This causes jquery to behave differently and not to eval scripts.

IgorMinar commented 10 years ago

@btford can you please investigate further.

btford commented 10 years ago

Yep, on it.

njs50 commented 10 years ago

jQuery 2.0 still executes script it just does it in a different way. i.e it executes the script then leaves the script tag in the DOM and flags it as executed (and won't execute it a second time on being moved around in the DOM). Previously it executed it and but didn't insert the script tag into the DOM.

The underlying problem is anything using animation code doesn't use jQuery or jQlite to insert the new nodes into the DOM anymore. i.e animate.enter() uses the native js insertBefore(), where previously it used $.after() or $.append().

I imagine the animation doesn't use jQuery to do the insert anymore for performance reasons...

btford commented 10 years ago

@njs50 is correct.

Here's a short test showing that jQuery does in fact eval the scripts in 2.0.3, but that elt.insertBefore (a la $animate.enter) does not eval the scripts (as we would expect): http://plnkr.co/edit/LrpjaCNLGNExuANGVdmm?p=preview

$animate.enter: https://github.com/angular/angular.js/blob/master/src/ng/animate.js#L107

We have a few choices now, assuming we want to support this:

  1. use jqLite.html() in $animate.enter() so this works like before
  2. write a decorator for $animate that changes the behavior of $animate.enter() for users who want this behavior
  3. provide a flag in $animateProvider that restores the old behavior

@matsko: was there a reason you avoided using jqLite.html() here? Seems like it could be for performance.

matsko commented 10 years ago

@btford what part exactly? Inside of ng/animate.js?

btford commented 10 years ago

Yes.

matsko commented 10 years ago

No reason. Using jqLite.html() should be fine.

rolbr commented 10 years ago

I silently followed this issue by email and while I didn't have a chance to test the fix yet, I wanted to say "Thanks!" to everybody who investigated, provided alternatives and finally worked on a fix. It's really much appreciated.

endorama commented 10 years ago

Thanks everybody! Is really nice too see this working again in such a clean way!

babujoym commented 9 years ago

@neemzy I have also run into the same problem. Could you show me how this works. The below url is now inaccessible http://www.dpaste.cc/paste/5279208b192d306411000003#javascript

neemzy commented 9 years ago

@babujoym TBH I don't really remember what I posted back then, but I guess it was something like :

angular.module('someModule', [])
    .directive('loadScript', function($filter) {
        return function(scope, element, attrs) {
            element.html('<script src="/path/to/script.js"></script>');
        };
    });

Then, at the location you initially wanted your script tag, use <div load-script></div> instead.

Again, this is not a perfect solution, but it has the undeniable advantage to just work for this kind of issues.