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

Inline source maps #2484

Merged
merged 7 commits into from
Apr 27, 2015
Merged

Inline source maps #2484

merged 7 commits into from
Apr 27, 2015

Conversation

mhegazy
Copy link
Contributor

@mhegazy mhegazy commented Mar 24, 2015

Support for inline source maps. As referenced in #2233, this change allows us to emit single file with source maps instead of having a separate file.

@mhegazy
Copy link
Contributor Author

mhegazy commented Mar 24, 2015

@frankwallis you might find this helpful.

@frankwallis
Copy link
Contributor

thanks!

@mhegazy
Copy link
Contributor Author

mhegazy commented Mar 25, 2015

Also pinging @teppeis and @basarat for typescript-simple that can use this change.

@@ -291,7 +293,7 @@ module ts {
function base64VLQFormatEncode(inValue: number) {
function base64FormatEncode(inValue: number) {
if (inValue < 64) {
return 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/'.charAt(inValue);
return base64Digits.charAt(inValue);
}
throw TypeError(inValue + ": not a 64 based value");
Copy link
Member

Choose a reason for hiding this comment

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

Maybe make this a Debug.fail

var charIndex = 0;

var length = input.length;
var i = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Use let consistently

@CyrusNajmabadi
Copy link
Contributor

Is the algorithm provided basically saying:

  1. We have a string.
  2. We want to consider the UTF8 encoding of that string.
  3. We want to take that encoding and convert it to Base64?

If so, i would personally prefer breaking out the two parts of this process. The UTF8 encoding, and the Base64 conversion.

Note: MDN already covers a good way to do this:
https://developer.mozilla.org/en-US/docs/Web/API/WindowBase64/btoa#Unicode_Strings

function utf8_to_b64( str ) {
  return window.btoa(unescape(encodeURIComponent( str )));
}
or
function b64EncodeUnicode(str) {
    return btoa(encodeURIComponent(str).replace(/%([0-9A-F]{2})/g, function(match, p1) {
        return String.fromCharCode('0x' + p1);
    }));
}

Now, if we don't have access to these functions in Node or Chakra, we could still do the above steps, just individually. i.e. break up a string into utf8 bytes. Then convert those bytes to Base64. I think this would be far easier to understand (and verify).

@mhegazy
Copy link
Contributor Author

mhegazy commented Mar 25, 2015

mm. let me try this out then. i remember there was a reason why i did not do that :)

@mhegazy mhegazy force-pushed the inlineSourceMaps branch from fda9974 to a998abb Compare April 8, 2015 17:57
@mhegazy
Copy link
Contributor Author

mhegazy commented Apr 8, 2015

So btoa is not available on node, and escape is not available on chakra :), so there is not an easy way to use these. I did split them into two functions, and added unit tests as well.

I have also handled --inlineSourceMap to be a parallel to --sourceMap instead of a dependent.

The reason covertToBase64 is in utilities is to allow for targeted unit tests. I have limited compiler baselines to limt the noise in .js files as the map is actually included in the .js file.

assert.equal(actual, expected, "Encoded string using convertToBase64 does not match buffer.toString('base64')");
}

it("Converts ASCII charaters correctelly", () => {
Copy link
Member

Choose a reason for hiding this comment

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

:%s/correctelly/correctly/g

@vladima
Copy link
Contributor

vladima commented Apr 25, 2015

LGTM

@vladima
Copy link
Contributor

vladima commented Apr 25, 2015

Can you also update transpile function to use inlineSources\inlineSourceMaps?

mhegazy added a commit that referenced this pull request Apr 27, 2015
@mhegazy mhegazy merged commit 5673660 into master Apr 27, 2015
@mhegazy mhegazy deleted the inlineSourceMaps branch April 27, 2015 17:47
@mhegazy mhegazy mentioned this pull request Apr 27, 2015
@mhegazy
Copy link
Contributor Author

mhegazy commented Apr 29, 2015

thanks @teppeis. fixed

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

Successfully merging this pull request may close these issues.

8 participants