Source url #766

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@jaubourg
Member
jaubourg commented May 6, 2012

http://bugs.jquery.com/ticket/11484

Not sure about this one. Adding the sourceURL parameter to globalEval was pretty straight-forward, as was adding the ajax options object as a second parameter to converters so that the "text script" converter in ajax/script.js is able to provide said sourceURL parameter.

However, when it comes to load, seeing as inline scripts need to refer to the loaded document as sourceURL, it's a bit hackish: has to pass through html then append then domManip. I'm particularly not happy with the change of signature of append.

@jaubourg
Member
jaubourg commented May 6, 2012

The only alternative to changing signatures in manipulation is to set some private data with the url on the target element, then unset it once the operation is done... and it seems a bit too heavy from a performance point of view for my taste.

@Krinkle
Member
Krinkle commented May 6, 2012

The ability to get the source URL of a script loaded through AJAX is very nice!

I've been waiting for this a long time, and I know I'm not the only one. I've heard time and time again that people hated that jQuery got its hands on the HTML, made an XHTTP request for <script src> tags it found, executed it in globalEval - thus loosing it in the DOM and loosing most source tracking, file paths/line number and other debugging abilities.

So definitely +1 on the parameter to $.globalEval and passing to it from within ajax.js.

And if I understand correctly, this way <script src> tags found inside HTML inserted with other functions (load(), append(), html(), before() etc.) will also automatically make use of this (without needing any new arguments in domManip or its callers).

However, I'm not so sure about the additional parameter added to domManip(). I assume the purpose of this is so that scripts that aren't loaded from a URL but are inline scripts (e.g. <script> foo( .. );</script>) detected via load(), append(), html(), before() etc. can also be given a source URL.

The problem with that last part, imho:

  • It is (currently) inconsistent. Only in html() and append(), not in the other domManip callers such as prepend(), before(), after() etc.
  • Adds a new parameter to these dom methods
    • can potentially do weird stuff since most of these dom methods accept multiple parameters already (e.g. append( $foo, bazElement, [quux], '<bar/>' );)
    • means we can't use that argument position for something else in the future. This way would not be for internal usage only, but actually be a valid debugging feature that applications will embed in their logic. So a new (document) api feature.
  • It would only be for debugging the source url of javascript inserted as inline scripts in html snippets inserted (indirectly) via domManip. Sounds like an uncommon case. And aside from that, none of the current scripts in the world would use this debugging feature because using it requires passing a parameter. And new scripts should probably do that in a different way anyway. Maybe via an option in an option object, or by executing the script directly (instead of passing it as inline script in HTML), by using a callback, or simply putting it in a file and using src.
@Krinkle
Member
Krinkle commented May 6, 2012
The only alternative to changing signatures in manipulation is to set some private data with the url on the target element, then unset it once the operation is done...

Not adding this feature for inline scripts would also solve the problem. So this new feature would only be for URL-referenced scripts (or scripts directly put into globalEval). I think that is the vast majority of the cases where people need source tracking, file paths/line numbers etc. for globalEval'ed scripts.

We could add support for inline scripts, one way or another, later if needed.

@jaubourg
Member
jaubourg commented May 6, 2012

@Krinkle I totally disagree with your assessment that inline scripts in injected html is an uncommon case. I'm convinced that it is the other way around. Everytime I saw a load with some javascript in it, it was always inlined. From what I gathered, people use this as a means to inject widgets and initialize them with a single glorious ajax request. Loading a script remotely from within a load seems in total opposition to the very goal.

To be honest, I truly hate load. I can't abide by the kind of patterns it can (and will) spawn (mainly mixing content and logic). But I also truly hate it when jQuery tries and dodges an issue for questions of principle. To (mis)quote Voltaire:

I do not agree with how you design your code, but I'll defend to the death your right to do it.

If we go and support sourceURL, let's make sure we do it for real, not just for cases that we like.

That being said, we all agree that changing the signature of append is not the way to go. As for changing the signature of html I personally think it makes sense (having a source URL comes with having source as text in my opinion), but I can see why one wouldn't feel comfortable limiting this use to html alone.

What are the other options (provided we try and support sourceURL for inline scripts, of course)?

  1. Setting a private data onto the element:

    // Before calling any manipulation method
    jQuery._data( element, "_sourceURL", sourceURL );
    // In domManip
    sourceURL = jQuery._data( element, "_sourceURL" );
    jQuery._removeData( element, "_sourceURL" );

    I just hate the fact that two separate piece of code share the ownership of some data attached to an element.

    I also dislike the idea of setting the information on the receiving end whereas the source URL comes from the injected content.

  2. Have some kind of scoping method that sets a variable within manipulation:

    jQuery.scope = function( url, actions ) {
     var previousURL = manipSourceURL; 
     manipSourceURL = url;
     actions();
     manipSourceURL = previousURL;
    };
    
    // Then, in domManip
    globalEval( code, manipSourceURL );
    
    // You'd use it like this:
    jQuery.scope( "path/to/my/url", function() {
     // Use append, prepend, html, whatever
     // and domManip will use manipSourceURL as the sourceURL
    });

I admit I kinda like the second method, even if it's a bit "much" for the feature itself.

@dmethvin
Member
dmethvin commented May 6, 2012

I'm not a fan of this, it seems to encourage bad practices and we're already in the process of deprecating things we don't want people to do. In particular, I think we should be making developers be more explicit about when they expect to be processing scripts. The whole XSS issue with $(html) is an example of the mess we run into when we try to be too magical. Just allowing jQuery.globalEval() and jQuery.parseHTML() to set sourceURL would be plenty for me. If that is a carrot to encourage people to use those APIs it seems like a win.

@dmethvin
Member

So @jaubourg did we come to the conclusion that it was sufficient to have this only for globalEval() and parseHTML() or whatever it ends up being named?

@jaubourg
Member

Let's do this. I'll just handle this in master directly when I have the time.

@jaubourg jaubourg closed this May 18, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment