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

Tests fail due to charset issue #36

Closed
jaredsmith opened this issue Feb 11, 2016 · 8 comments
Closed

Tests fail due to charset issue #36

jaredsmith opened this issue Feb 11, 2016 · 8 comments

Comments

@jaredsmith
Copy link

I'm running the self-tests as part of an effort to get this module packaged as an RPM for Fedora, but three tests are failing due to charset issues.

From a very brief analysis, it seems that the fromComment() function is ignoring the character set on the comment, and that the toComment() function isn't adding the charset back in.

Here's a very simple program to explain:

generator = require('inline-source-map')
convert = require('convert-source-map')

var gen = generator()
    .addMappings('foo.js', [{ original: { line: 2, column: 3 } , generated: { line: 5, column: 10 } }], { line: 5 })
    .addGeneratedMappings('bar.js', 'var a = 2;\nconsole.log(a)', { line: 23, column: 22 })

  , comment = gen.inlineMappingUrl()

console.log('comment: %s',comment);
console.log('converted: %s',convert.fromComment(comment).toComment());

The output is as follows:

comment: //# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbImZvby5qcyIsImJhci5qcyJdLCJuYW1lcyI6W10sIm1hcHBpbmdzIjoiOzs7Ozs7Ozs7VUFDRzs7Ozs7Ozs7Ozs7Ozs7c0JDREg7c0JBQ0EiLCJmaWxlIjoiIiwic291cmNlUm9vdCI6IiJ9
converted: //# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbImZvby5qcyIsImJhci5qcyJdLCJuYW1lcyI6W10sIm1hcHBpbmdzIjoiOzs7Ozs7Ozs7VUFDRzs7Ozs7Ozs7Ozs7Ozs7c0JDREg7c0JBQ0EiLCJmaWxlIjoiIiwic291cmNlUm9vdCI6IiJ9

As you can see, the initial comment contains the string "charset=utf-8", but after the conversion the charset is missing from the resulting comment.

@thlorenz
Copy link
Owner

As you can see all tests are passing on travis, so I the covered features work.

The example you posted is not supposed to work .. charsets are not conserved during conversion.

However I agree with you that .toComment should better include the correct charset.
Do you mind adding a failing test (derived from your example above) and fix the issue via a PR so that test passes again? Should just need a simple change around here
I personally don't currently have the bandwidth to do it.

Thanks.

@RossGammon
Copy link

I have just bumped into the same problem trying to package the latest convert-source-map for Debian. The test can be made to fail on Travis by bumping the inline-source-map dependency up to 0.6.1:
https://travis-ci.org/RossGammon/convert-source-map/jobs/156038396

It will take me a little while to workout what the simple change should be in index.js though :-)
I am not sure how the correct charset can be derived so that it can be inserted into the data string. But my javascript skills are very slowly improving. Any further hints?

@thlorenz
Copy link
Owner

Sorry can't help you much, but I'd love a PR that shows the problem via a test and then fixes it :)

@thlorenz
Copy link
Owner

Fixed with #51

@scamden
Copy link

scamden commented Mar 3, 2017

@thlorenz sadly.. having charset in inline source map is broken in webpack source map loader.. not sure if you view it as their issue, but just so you know this latest change breaks usage of babel with webpack among other things probably.

@thlorenz
Copy link
Owner

thlorenz commented Mar 4, 2017

Sorry to hear that @scamden.
I do think this is a webpack issue as having the charset there follows the standard.
Will it be hard for the webpack eco system to fix things and then publish a new working version?

However is there anything we can do here to help? It'd be pretty drastic, but we could fix webpack by unpublishing the minor upgrade from npm and republishing this as a major upgrade (even though I think we shouldn't need to since there weren't any API changes).
I'd need to be convinced that this is really necessary and that the webpack folks can't fix things in a timely manner, but I'm open to it if absolutely needed.

@scamden
Copy link

scamden commented Mar 4, 2017

That's super cool of you! There actually is a PR to fix the issue over there, but it hasn't seen much activity for a while, due partially to the contributor failing to sign the CLA so far. I'm hoping it might get more attention now that such a major tool breaks with them. The pr is here: webpack-contrib/source-map-loader#21 (comment)

Thanks again for a rad response to something that's not even your issue :)

@thlorenz
Copy link
Owner

thlorenz commented Mar 4, 2017

Ah nice looks like the fix is under way and simple enough ... don't think we need to do anything here then.
I suppose even if the PR doesn't make it, someone could apply a hotfix in no time.

Good luck!

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

4 participants