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

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

Closed
kamstrup opened this issue Aug 26, 2013 · 41 comments
Closed

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

kamstrup opened this issue Aug 26, 2013 · 41 comments

Comments

@kamstrup
Copy link

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
Copy link

@kamstrup
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Contributor

matsko commented Aug 29, 2013

Taking a look in the morning tomorrow.

@njs50
Copy link

njs50 commented Sep 12, 2013

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
Copy link
Contributor

matsko commented Sep 12, 2013

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

@njs50
Copy link

njs50 commented Sep 12, 2013

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
Copy link
Contributor

olostan commented Sep 18, 2013

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
Copy link
Contributor

btford commented Sep 24, 2013

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?

@ghost ghost assigned btford Sep 24, 2013
@btford
Copy link
Contributor

btford commented Sep 24, 2013

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.

@btford btford closed this as completed Sep 24, 2013
@njs50
Copy link

njs50 commented Sep 26, 2013

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
Copy link
Contributor

olostan commented Sep 27, 2013

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
Copy link

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
Copy link
Contributor

btford commented Oct 14, 2013

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

@endorama
Copy link

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

:) Thanks

@njs50
Copy link

njs50 commented Oct 14, 2013

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 [email protected]:

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//issues/3756#issuecomment-26279411
.

@rolbr
Copy link

rolbr commented Oct 20, 2013

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
Copy link

neemzy commented Oct 29, 2013

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
Copy link

endorama commented Nov 5, 2013

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
Copy link
Contributor

olostan commented Nov 5, 2013

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

@neemzy
Copy link

neemzy commented Nov 5, 2013

@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
Copy link

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
Copy link

is this issue going to be handle soon?

@neemzy
Copy link

neemzy commented Nov 14, 2013

@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
Copy link

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
Copy link

neemzy commented Nov 14, 2013

@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
Copy link

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
Copy link

neemzy commented Nov 14, 2013

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
Copy link

@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
Copy link
Contributor

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 IgorMinar reopened this Nov 19, 2013
@IgorMinar
Copy link
Contributor

@btford can you please investigate further.

@btford
Copy link
Contributor

btford commented Nov 19, 2013

Yep, on it.

@njs50
Copy link

njs50 commented Nov 19, 2013

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
Copy link
Contributor

btford commented Nov 19, 2013

@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
Copy link
Contributor

matsko commented Nov 19, 2013

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

@btford
Copy link
Contributor

btford commented Nov 19, 2013

Yes.

@matsko
Copy link
Contributor

matsko commented Nov 19, 2013

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

btford added a commit to btford/angular.js that referenced this issue Nov 20, 2013
In 1.2, the behavior of ngInclude was modified to use DOM APIs rather than jqLite. This means that
even when jQuery was loaded, ngInclude was not calling into it, and thus scripts were not eval'd
as they had been before. Although the use of ngInclude to eval scripts as a lazy-loading strategy
was never an intentional feature, this patch restores the ability to do so.

Closes angular#3756
@btford btford closed this as completed in c47abd0 Nov 20, 2013
@rolbr
Copy link

rolbr commented Nov 21, 2013

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
Copy link

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

jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
In 1.2, the behavior of ngInclude was modified to use DOM APIs rather than jqLite. This means that
even when jQuery was loaded, ngInclude was not calling into it, and thus scripts were not eval'd
as they had been before. Although the use of ngInclude to eval scripts as a lazy-loading strategy
was never an intentional feature, this patch restores the ability to do so.

Closes angular#3756
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
In 1.2, the behavior of ngInclude was modified to use DOM APIs rather than jqLite. This means that
even when jQuery was loaded, ngInclude was not calling into it, and thus scripts were not eval'd
as they had been before. Although the use of ngInclude to eval scripts as a lazy-loading strategy
was never an intentional feature, this patch restores the ability to do so.

Closes angular#3756
@babujoym
Copy link

babujoym commented Mar 8, 2015

@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
Copy link

neemzy commented Mar 8, 2015

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

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

No branches or pull requests