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

Fix charset in inline data url sourcemaps #2856

Closed
wants to merge 1 commit into from

Conversation

tadeegan
Copy link
Contributor

I noticed that babel produces inline source maps of the form:

//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbImZyb250ZW5kL2FwcC9yZWFjdC1jb21wb25lbnRzL2FwcC5qc3giXSwibmFtZXMiOltdLCJtYXBwaW5ncyI6IkFBQUE7Ozs7QUFJQSxTQUFTLE9BQVQsUUFBd0IsWUFBeEI7QUFDQSxPQUFPLEdBQVAsTUFBZ0IsVUFBaEI7QUFDQSxPQUFPLFlBQVAsTUFBeUIsbUJBQXpCO0FBQ0EsU0FBUyxTQUFULFFBQTBCLGFBQTFCO0FBQ0EsT0FBTyxNQUFQLE1BQW1CLFdBQW5COztBQUVBO0FBQ0EsTUFBTSxlQUFOLFNBQThCLE1BQU0sU0FBcEMsQ0FBOEM7QUFDMUM7QUFDQSxnQkFBWSxLQUFaLEVBQW1CLE9BQW5CLEVBQTRCO0FBQ3hCLGNBQU0sS0FBTixFQUFhLE9BQWI7QUFDQSxhQUFLLEtBQUwsR0FBYTtBQUNULDJCQUFlLGVBRE4sRUFDdUI7QUFDaEMsNkJBQWlCO0FBQ2IsMEJBQVU7QUFERyxhQUZSO0FBS1QsMkJBQWU7QUFDWCwwQkFBVTtBQURDLGFBTE47QUFRVCw0Q0FBaUMsSUFBSSxJQUFKLEVBQUQsQ0FBVyxPQUFYLEVBUnZCLENBUTZDO0FBUj......

This is not recognized by the SourceMapResolver because of the 'charset=utf-8'

See acceptable data url format: https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs

if (md.equals(metadata)) {
byte[] data = BaseEncoding.base64().decode(
url.substring(base64StartIndex + BASE64_START.length()));
return new String(data, StandardCharsets.UTF_8);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Let's static-import UTF_8 and remove StandardCharsets. here.

* Based on https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/Data_URIs
* @param url
* @return String or null.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add @Nullable annotation since we explicitly expect this to return null in some cases.

@MatrixFrog
Copy link
Contributor

otherwise lgtm

@tadeegan
Copy link
Contributor Author

done

@MatrixFrog
Copy link
Contributor

Can you squash this into a single commit as well? (We have a bug open to fix our script so it can accept multi-commit PRs but I don't expect we'll actually fix it anytime soon, if I'm being honest)

@tadeegan
Copy link
Contributor Author

done.

but I don't expect we'll actually fix it anytime soon, if I'm being honest

classic

@brad4d brad4d closed this in 80ee3a4 Mar 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants