Skip to content

Fix inline source-map support with charset in the data url #65

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

Closed
wants to merge 1 commit into from

Conversation

nopnop
Copy link

@nopnop nopnop commented May 25, 2015

Inline source map prefix should contain an charset:
data:application/json;charset:utf-8;base64,

This is the case with the last releases of browserify for instance.
This fix make inline source map detection more flexible to support
this kind of data url (not only with charset) and use a regexp instead
of a static string: /^data:application\/json[^,]+base64,/

(I didn’t push the generated browser-source-map-support.js with this
PR, do you want me to ?)

Inline source map prefix should contain an charset:
`data:application/json;charset:utf-8;base64,`

This is the case with the last releases of browserify for instance.
This fix make inline source map detection more flexible to support 
this *kind* of data url (not only with charset) and use a regexp instead
of a static string: `/^data:application\/json[^,]+base64,/`

(I didn’t push the generated `browser-source-map-support.js` with this
PR, do you want me to ?)
@julien-f
Copy link
Collaborator

Thank you for this PR.
Could you add some tests please?

And no, do not push a new build, we only do that before a release to keep the PRs as clean as possible.

@nopnop
Copy link
Author

nopnop commented May 25, 2015

Hello,
I tried, I which I could, but:

  • currently many tests does not pass on master with a clean npm install (failures: 6/19)
  • browserify (3.44.2 -> 10.2.1) and source-map (0.1.32 -> 0.4.2) are very outdated - and almost all test fail after updating them
  • This test seems to exist already "sourcesContent with data URL", but I can't test this without updating browserify and source-map
  • After updating browserify and source-map, the builded file browserify-test/compile.js contain a data url with charset:utf-8 as described in my PR : maybe its enough to consider that this test should not fail ?

I tried to fix the tests... but there is to many small side-effect out of the range of this PR: I will be happy to provide a test for this PR if needed, but may I suggest that you update dependencies and tests first ?

@julien-f
Copy link
Collaborator

I personally do not have the time to do that for now and I think it would be unreasonable to upgrade browserify and source-map before the next release (which, I hope, will be published this week).

@LinusU
Copy link
Collaborator

LinusU commented Jun 16, 2015

All tests pass for me on OS X with node v0.12.4 with clean git clone and npm install.

@nopnop nopnop closed this Jun 16, 2015
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

Successfully merging this pull request may close these issues.

3 participants