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

Remove sourcemap linking with a `//#` comment #2385

Closed
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@mquandalle
Contributor

mquandalle commented Aug 7, 2014

Firefox now supports sourcemap linking using a HTTP response header
Firefox print a lot of warning when using the //# linking

@mquandalle mquandalle changed the title from Drop sourcemap linking with a `//#` comment to Remove sourcemap linking with a `//#` comment Aug 7, 2014

@glasser

This comment has been minimized.

Member

glasser commented Aug 15, 2014

Thanks for noticing!

This looks incomplete:

  • We should update the comment in webapp_server.js too.
  • We should also get rid of the horrible hack in JsImage.write which tries to undo the effect of doing this transformation twice (see 9053986).

Additionally, this is a good time figure out:

  • We should also figure out: should we be setting SourceMap or X-SourceMap? We currently set X-SourceMap, which I believe was not the proposed standard but was the thing that Chrome actually supported at the time.. What do current Chrome and FF support?
  • Are there any other browsers (Safari?) that now support source maps that we should be testing too?

Can you make these updates and research the other issues? Definitely would love to get this cleanup in (perhaps after 0.9.0).

mquandalle added some commits Aug 7, 2014

Remove sourcemap linking with a `//#` comment
Firefox now supports sourcemap linking using a HTTP response header
Firefox print a lot of warning when using the `//#` linking
@mquandalle

This comment has been minimized.

Contributor

mquandalle commented Aug 18, 2014

Thanks for the review @glasser. I've updated my PR to remove dead code & comments, and to use the new recommended source map header. It seems that Safari 7 and IE11 both support source maps but I haven't had a chance to verify it since I'm a happy linux user :)

@mquandalle

This comment has been minimized.

Contributor

mquandalle commented Sep 3, 2014

I hope this will be merged soon, here is how the firefox console looks like for any meteor project:

firefox-source-map-meteor

@glasser

This comment has been minimized.

Member

glasser commented Sep 4, 2014

I tested this with FF 32 and your second commit (changing from X-SourceMap to SourceMap) seems to have broken it. Are you sure it works on FF with SourceMap?

It did seem to work on Chrome and Safari with either header. So let's just take your first commit, and leave X-SourceMap with a different comment.

@glasser

This comment has been minimized.

Member

glasser commented Sep 4, 2014

Merged.

@Slava

This comment has been minimized.

Slava commented on tools/bundler.js in 431af48 Sep 30, 2014

I am reverting this part because it breaks the server-side source-mapping

Slava added a commit that referenced this pull request Sep 30, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment