Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Source Map doesn't work on Mozilla Firefox #159

Closed
tortuetorche opened this issue Jul 25, 2014 · 16 comments
Closed

Source Map doesn't work on Mozilla Firefox #159

tortuetorche opened this issue Jul 25, 2014 · 16 comments

Comments

@tortuetorche
Copy link

If we remove the XSSI protection here:
https://github.com/nodeca/mincer/blob/1.1.1/lib/mincer/server.js#L301
And here:
https://github.com/nodeca/mincer/blob/1.1.1/lib/mincer/manifest.js#L264
Source Map is OK with the red panda.

Maybe someone know an other XSSI protection which is compatible with Chrome and Firefox ?

@tortuetorche tortuetorche changed the title Source-map doesn't work on Mozilla Firefox Source-maps doesn't work on Mozilla Firefox Jul 25, 2014
@puzrin
Copy link
Contributor

puzrin commented Jul 25, 2014

Hm... interesting info. Didn't knew that it was reason of FF fail.

@tortuetorche tortuetorche changed the title Source-maps doesn't work on Mozilla Firefox Source maps doesn't work on Mozilla Firefox Jul 26, 2014
@tortuetorche tortuetorche changed the title Source maps doesn't work on Mozilla Firefox Source Map doesn't work on Mozilla Firefox Jul 26, 2014
@tortuetorche
Copy link
Author

@puzrin I've found the solution:
According to this post: https://groups.google.com/forum/#!topic/mozilla.dev.js-sourcemap/3QBr4FBng5g

We just need to add a single quote ' to the protection string: )]}' [newline].

So it looks like this:

)]}'
{"version":3,"sources":["laravel_larasset_app/app/assets/javascripts/application.js.coffee"],"names":[],"mappings":"AAAA;;AAAA,WAAY,IAAZ;;AAAA,OAEO,CAAC,GAAR,CAAY,QAAZ,CAFA","sourcesContent":["longName  = \"yo\"\n\nconsole.log(longName) # debug\n\n"],"sourceRoot":"/"}

I can create a pull request if you want, just let me know.

Have a good day,
Tortue Torche

@puzrin puzrin closed this as completed in e99844c Jul 26, 2014
@puzrin
Copy link
Contributor

puzrin commented Jul 26, 2014

Please, check if version from master is ok. If your problem is solved, i'll publish it.

@tortuetorche
Copy link
Author

@puzrin 👍 It's fixed in both server mode and manifest compiler.
Many thanks Vitaly !

@tortuetorche
Copy link
Author

I can also confirm that JavaScript Source Mapping works with Internet Explorer 11 (IE11).
I don't know if it was the case before this commit e99844c ...

Here some useful docs: http://msdn.microsoft.com/en-US/library/ie/dn255007%28v=vs.85%29#source_maps

@puzrin
Copy link
Contributor

puzrin commented Jul 28, 2014

Thanks

@ricardograca
Copy link

Is there anything special that needs to be done to make this work? I have the environment.enable('source_maps') line, and I do see a line like this in my main manifest file:

/*# sourceMappingURL=main-29c6f83c314bb8244735632f0ce6540f.css.map */

but on the Firefox console the line numbers are the ones from main.css only (the manifest) and even the style definitions point to main.css instead of the expected original file. It doesn't matter if I use development or production environment.

@tortuetorche
Copy link
Author

You're right CSS source mapping looks broken with Firefox 33.x.x when assets are precompiled (via a manifest file).

@tortuetorche
Copy link
Author

With every Web browsers (Chrome included) CSS source mapping is buggy when assets are compressed.
Because you're going to just saw one CSS source file which is the concatenation of all your CSS/LESS/Sass... files.

@puzrin
Copy link
Contributor

puzrin commented Nov 21, 2014

May be css needs inline comment instead of block one? Or needs to have XSSI header removed?

//# sourceMappingURL=

We don't use sourcemaps at all, and i didn't digged this area too much.

@tortuetorche
Copy link
Author

As I know, inline comments aren't supported in CSS, but you can do it in Less.
According to my tests XSSI header isn't an issue.

So mincer need a fix to support CSS source mapping in production mode (when CSS files are concatenated via a manifest file) with Firefox.

You can fix it manually: remove all sourceMappingURL= comments except the last one in your main CSS concatenated file.

E.G. The application-dc2eda33eab77f6f0a11217e03088820.css file:

/*# sourceMappingURL=application.css.map */
/*CSS rules...*/
/*# sourceMappingURL=bootstrap.less.map */
/*CSS rules...*/
/*# sourceMappingURL=application-dc2eda33eab77f6f0a11217e03088820.css.map */

Become:

/*CSS rules...*/
/*# sourceMappingURL=application-dc2eda33eab77f6f0a11217e03088820.css.map */

Now Firefox is able to get every CSS/LESS/Sass files of your CSS manifest.
This workaround is also compatible with Chrome and by the way I just realized that IE11 doesn't support yet CSS source mapping, c'mon Microsoft!

@puzrin
Copy link
Contributor

puzrin commented Nov 21, 2014

Do i understand rigth, that mincer forgets to clear sourcemap comments in js & css prior to concatenate assets?

@tortuetorche
Copy link
Author

I don't know if mincer (or its dependencies) keep them intentionally or not.
But for CSS files, yes it leaves sourcemaps comments, for JS files they're cleared.

Sorry for my bad English, I'm French.

@puzrin
Copy link
Contributor

puzrin commented Nov 21, 2014

Created new issue #168

Could you investigate? I see 2 potentioal problems:

  1. Comment format for CSS can be wrong https://github.com/nodeca/mincer/blob/master/lib/mincer/assets/bundled.js#L204 (may be, it should be the same as for JS).
  2. We don't remove any comments at all in concatenator https://github.com/nodeca/mincer/blob/master/lib/mincer/assets/bundled.js#L46 . Probably, sourcemap lib do it automatically for /*# */ and do nothing fo //#

@tortuetorche
Copy link
Author

You means

Probably, sourcemap lib do it automatically for //# and do nothing for /*# */

I guess.

@puzrin
Copy link
Contributor

puzrin commented Nov 21, 2014

Yes. I don't have full browser set to dig deep. If you have some time - try to change comments for generated files, and let me know if this improve something.

Code to change is here https://github.com/nodeca/mincer/blob/master/lib/mincer/assets/bundled.js#L204

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

No branches or pull requests

3 participants