Bug Tracker

Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#13274 closed bug (fixed)

IE < 9: "'jquery' is undefined" w/ sourceMappingURL comment

Reported by: alexc223@… Owned by: alexc223@…
Priority: blocker Milestone: 1.9.1
Component: build Version: 1.9.0
Keywords: Cc: paul.irish
Blocked by: Blocking:

Description

When using MSIE < 9 and jQuery 1.9.0 I am getting the following warning/error:

'jquery' is undefined

This error is coming from line 4 in the minified versions, which is (a comment!):

//@ sourceMappingURL=jquery.min.map

Change History (25)

comment:1 Changed 11 years ago by dmethvin

Owner: set to alexc223@…
Status: newpending

Can you provide a test page that we can use? I include *just* the minified 1.9.0 and it seems to work fine.

comment:2 Changed 11 years ago by chris@…

I can confirm this bug. You can see it live on http://www.achievementstats.com/

comment:3 Changed 11 years ago by dmethvin

Component: unfiledbuild
Milestone: None1.9.1
Priority: undecidedblocker
Status: pendingopen

It definitely is happening on those sites, although I can't reproduce it with this simple jsfiddle: http://jsfiddle.net/dmethvin/vvtJN/3/

If you get into the IE8 debugging console and type //@ sourceMappingURL=jquery.min.map you see the same error as above though, so it does seem real. We just need to figure out what conditions cause it.

comment:4 Changed 11 years ago by dmethvin

I am starting to feel sick...it's conditional code.

http://www.javascriptkit.com/javatutors/conditionalcompile.shtml

If someone, anyone, uses //@cc_on before jQuery loads it looks like the sourceMappingURL line will cause an error in IE8.

http://imgur.com/x2RL8KJ

Supposedly it can be turned off with @cc_off but that would have to be in OUR source file for defensive purposes and would affect following code.

comment:5 Changed 11 years ago by anonymous

This will affect *all versions* of IE, not just IE8. See this fiddle in IE10 or below.

comment:6 Changed 11 years ago by paul.irish

Okay spent some time on this with Jake Archibald...

This is where WebKit parses the comment. It does NOT need to start on a newline, but there are some other specifics in the regex it's looking for: https://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/inspector/ContentSearchUtils.cpp&exact_package=chromium&q=sourceMappingURL&type=cs&l=143

Two possible fixes...

jQuery-side fix..

Augment the current reference with flowerbox comments (on new lines).

/*
//@ sourceMappingURL=undeclaredIdentifier
*/

cc_on's will still work (and so will the sourcemapping), but IE won't try to parse this as a CC directive.

User-side fix

Since this is only repro'd when a page turns on conditional compilation before including jQuery, DONT DO THAT.

  1. Don't use @cc_on
  2. Include jQuery before one of your scripts turns on @cc_on
  3. stop using some super old legacy version of the html5shiv and update to the latest version
  4. Don't use @cc_on. Use this instead https://gist.github.com/527683
  5. @cc_on dramatically slows down js evaluation. don't use it.

comment:7 Changed 11 years ago by alexc223@…

Yep, I can confirm that's the case - an old html5shiv was in use, which uses conditional compilation. Upgrading to the latest html5shiv (which no longer uses it), has resolved this for me.

comment:8 Changed 11 years ago by chris@…

AchStats.com uses this table sort script: http://www.frequency-decoder.com/2006/09/16/unobtrusive-table-sort-script-revisited It has indeed conditional code, but that's nothing we could change. However loading jQuery before loading the sort script has resolved the issue! Thanks for the research. :)

comment:9 Changed 11 years ago by gibson042

As it turns out, we can fix this and even save a byte: https://github.com/jquery/jquery/pull/1143

Whether we want to remains an open question.

comment:10 Changed 11 years ago by Richard Gibson

Resolution: fixed
Status: openclosed

Fix #13274: Wrap sourceMap directive in multiline comments. Close gh-1143.

Changeset: ac93559eb9f18fcaec95dfdc97358b1b85bfe234

comment:11 Changed 11 years ago by Richard Gibson

Fix #13274: Wrap sourceMap directive in multiline comments. Close gh-1143. (cherry picked from commit ac93559eb9f18fcaec95dfdc97358b1b85bfe234)

Changeset: 487b703521e63188102c73e8ce6ce203d28f260b

comment:12 Changed 11 years ago by Matt Robenolt <matt@…>

I just noticed now in master that the //@ sourceMappingURL= indicator is now at the top of the document.

According to the version 3 spec for source maps, it explicitly says it should be at the end of the document. I'm not sure how these are implemented at the browser level, but could it cause any issues? Or should this idea be brought up to browser devs and have the spec loosened?

Spec document: https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k/edit#heading=h.txk3vdsh99hf

Compiled source from master: https://gist.github.com/raw/43037ab16b9b22d1ceb0/gistfile1.js

comment:13 Changed 11 years ago by gibson042

I observe that particular statement to be in the "Conventions" section, and also that a strict reading would prohibit the trailing */ required by this ticket. Given that our current approach was specifically crafted to match browser implementation, who do we talk to about updating the spec?

comment:14 Changed 11 years ago by dmethvin

Cc: paul.irish added

Probably good to pull in Paul Irish here. I am not at all a fan of the current "convention" of using a newline-terminated comment at the end of the file. It also makes more sense for the comment to be a the top of the file. But we should get all the parties involved to agree on what is within the spec.

While we're changing the spec it would be great to pick a sequence that wasn't already in use by @cc_on.

comment:15 Changed 11 years ago by Matt Robenolt <matt@…>

According to the source of both WebKit and FF, this will technically work. Both work by just looking for a comment that fits that pattern, which technically could be left open to the sourceMappingURL indicator being *anywhere* in the document.

(Slightly out of the scope of this discussion), but I think the beginning of the document makes a lot of sense for implementations other than a browser for one good reason:

It's possible to stream in bytes and detect a source map without loading the whole file into memory. If the source map could be detected early, you can disregard the rest and just fetch the source.

Granted, it's a small idea, but it makes sense to me. Or just strictly use the proposed SourceMap header, which seems the cleanest of them all.

comment:16 Changed 11 years ago by dmethvin

Using the HTTP header may seem like a good idea but isn't. Most web devs can change the content of a file with no problem but can't change the headers sent with the file without access to server configuration. It also won't work if the file is used locally.

comment:17 Changed 11 years ago by mattrobenolt

@dmethvin Good point. It is harder to maintain a header. So maybe just a preference for a header if possible. :)

comment:18 Changed 11 years ago by adam.dorsey@…

I still get an error in IE8, even when using the fix:

/* @ sourceMappingURL=undeclaredIdentifier */

comment:19 Changed 11 years ago by adam.dorsey@…

Sorry, here it is in a proper code block:

/*
//@ sourceMappingURL=undeclaredIdentifier
*/

comment:20 in reply to:  6 Changed 10 years ago by meelash

Paul, do you have any numbers/tests/explanation for why turning on cc slows down js? Is this in IE only (given that other browser should be ignoring it as a comment)?

Replying to paul.irish:

Okay spent some time on this with Jake Archibald...

This is where WebKit parses the comment. It does NOT need to start on a newline, but there are some other specifics in the regex it's looking for: https://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/inspector/ContentSearchUtils.cpp&exact_package=chromium&q=sourceMappingURL&type=cs&l=143

Two possible fixes...

jQuery-side fix..

Augment the current reference with flowerbox comments (on new lines).

/*
//@ sourceMappingURL=undeclaredIdentifier
*/

cc_on's will still work (and so will the sourcemapping), but IE won't try to parse this as a CC directive.

User-side fix

Since this is only repro'd when a page turns on conditional compilation before including jQuery, DONT DO THAT.

  1. Don't use @cc_on
  2. Include jQuery before one of your scripts turns on @cc_on
  3. stop using some super old legacy version of the html5shiv and update to the latest version
  4. Don't use @cc_on. Use this instead https://gist.github.com/527683
  5. @cc_on dramatically slows down js evaluation. don't use it.

comment:21 Changed 10 years ago by anonymous

Will this be fixed in a future version? I have web essentials installed which will automatically change my minified files back to the single line comment. The workaround of a multiline flower comment does not work with Web Essentials.

comment:22 Changed 10 years ago by dmethvin

On our end, the only option we have is to remove support for sourcemaps altogether.

Your tools should not be mangling that comment, I can't think of a reason why they would do that. If you are combining/(re)compressing files the comment becomes useless anyway because the map no longer represents the new file. Remove it or remove it not, there is no mangle.

comment:23 in reply to:  22 Changed 10 years ago by anonymous

Replying to dmethvin:

On our end, the only option we have is to remove support for sourcemaps altogether.

Your tools should not be mangling that comment, I can't think of a reason why they would do that. If you are combining/(re)compressing files the comment becomes useless anyway because the map no longer represents the new file. Remove it or remove it not, there is no mangle.

What I'm saying is that Web Essentials automatically creates the minfied file and map file when you save the unminified file. So if I save the unminified file, the minified file gets re-created with the single line comment.

comment:24 Changed 10 years ago by dmethvin

Well that's not jQuery's issue at all, Web Essentials is creating an incompatible file. You'll need to let the Web Essentials people know they're generating a file that causes errors in IE and/or convince the sourcemap folks to adopt a syntax that is compatible with IE's @cc_on. I haven't had much luck with the latter myself.

comment:25 Changed 10 years ago by m_gol

@dmethvin I've talked to Nick Fitzgerald at the Front Trends conference in Warsaw recently and he's open to changing the @ symbol, in his presentation about source maps he even directly said that it'll probably change. I'm optimistic about that.

Note: See TracTickets for help on using tickets.