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

source map support #50

Merged
merged 2 commits into from Sep 5, 2012

Conversation

Projects
None yet
4 participants
@michaelficarra
Member

michaelficarra commented Sep 4, 2012

This adds source map support to escodegen through use of the Mozilla Source Map library.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Sep 4, 2012

This pull request fails (merged 1312afc4 into ab43e95).

travisbot commented Sep 4, 2012

This pull request fails (merged 1312afc4 into ab43e95).

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Sep 4, 2012

Member

@travisbot: The comment tests are failing. That's due to the code marked FIXME.

Member

michaelficarra commented Sep 4, 2012

@travisbot: The comment tests are failing. That's due to the code marked FIXME.

@Constellation

This comment has been minimized.

Show comment
Hide comment
@Constellation

Constellation Sep 4, 2012

Member

Great work!!
I'll see code :)

Member

Constellation commented Sep 4, 2012

Great work!!
I'll see code :)

@Constellation

This comment has been minimized.

Show comment
Hide comment
@Constellation

Constellation Sep 4, 2012

Member

I've created pull request for your branch. michaelficarra#1

Member

Constellation commented Sep 4, 2012

I've created pull request for your branch. michaelficarra#1

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra
Member

michaelficarra commented Sep 4, 2012

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Sep 5, 2012

Member

Only one failing test remains, and this is due to running SourceNode.prototype.replaceRight on a SourceNode that looks like

{ children: [ '//A\n', '', '' ],
  line: null,
  column: null,
  source: undefined }

Since the rightmost strings are empty, replacing \s+$ does nothing. I'm going to change the mozilla source map code to ignore empty strings, guaranteeing that the rightmost string is non-empty.

Member

michaelficarra commented Sep 5, 2012

Only one failing test remains, and this is due to running SourceNode.prototype.replaceRight on a SourceNode that looks like

{ children: [ '//A\n', '', '' ],
  line: null,
  column: null,
  source: undefined }

Since the rightmost strings are empty, replacing \s+$ does nothing. I'm going to change the mozilla source map code to ignore empty strings, guaranteeing that the rightmost string is non-empty.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Sep 5, 2012

Member

Still TODO: I need to find a more efficient way to test the generated string than toSourceNode(X).toString() (or avoid the tests).

Member

michaelficarra commented Sep 5, 2012

Still TODO: I need to find a more efficient way to test the generated string than toSourceNode(X).toString() (or avoid the tests).

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Sep 5, 2012

Member

With mozilla/source-map#19 and now mozilla/source-map#20, all tests are passing.

Member

michaelficarra commented Sep 5, 2012

With mozilla/source-map#19 and now mozilla/source-map#20, all tests are passing.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Sep 5, 2012

Member

Alright, this LGTM. Usage with source map generation will have to wait until those mozilla/source-map pulls are merged, but it's working fine as-is. Let's 🚢 it.

Future work: optimise toSourceNode(X).toString() usage depending on how it is used.

Member

michaelficarra commented Sep 5, 2012

Alright, this LGTM. Usage with source map generation will have to wait until those mozilla/source-map pulls are merged, but it's working fine as-is. Let's 🚢 it.

Future work: optimise toSourceNode(X).toString() usage depending on how it is used.

@Constellation

This comment has been minimized.

Show comment
Hide comment
@Constellation

Constellation Sep 5, 2012

Member

Alright, this LGTM. Usage with source map generation will have to wait until those mozilla/source-map pulls are merged, but it's working fine as-is. Let's it.

Yes, I think so too. Merging this. Thanks for your great contribution :)

Future work: optimise toSourceNode(X).toString() usage depending on how it is used.

NOTE: In my environment (node version 0.8.8), npm test result is following.

  • master branch
    test: 312 tests. 0 failures. 72 ms
    api: 7 tests. 0 failures. 1 ms
    options: 468 tests. 0 failures. 92 ms
    comment: 18 tests. 0 failures. 13 ms
    compare: 6 tests. 0 failures. 11 ms
    identity: 11 tests. 0 failures. 11064 ms
  • source-maps branch
    test: 312 tests. 0 failures. 77 ms
    api: 7 tests. 0 failures. 1 ms
    options: 468 tests. 0 failures. 103 ms
    comment: 18 tests. 0 failures. 17 ms
    compare: 6 tests. 0 failures. 14 ms
    identity: 11 tests. 0 failures. 13038 ms
Member

Constellation commented Sep 5, 2012

Alright, this LGTM. Usage with source map generation will have to wait until those mozilla/source-map pulls are merged, but it's working fine as-is. Let's it.

Yes, I think so too. Merging this. Thanks for your great contribution :)

Future work: optimise toSourceNode(X).toString() usage depending on how it is used.

NOTE: In my environment (node version 0.8.8), npm test result is following.

  • master branch
    test: 312 tests. 0 failures. 72 ms
    api: 7 tests. 0 failures. 1 ms
    options: 468 tests. 0 failures. 92 ms
    comment: 18 tests. 0 failures. 13 ms
    compare: 6 tests. 0 failures. 11 ms
    identity: 11 tests. 0 failures. 11064 ms
  • source-maps branch
    test: 312 tests. 0 failures. 77 ms
    api: 7 tests. 0 failures. 1 ms
    options: 468 tests. 0 failures. 103 ms
    comment: 18 tests. 0 failures. 17 ms
    compare: 6 tests. 0 failures. 14 ms
    identity: 11 tests. 0 failures. 13038 ms

Constellation added a commit that referenced this pull request Sep 5, 2012

@Constellation Constellation merged commit 181dea1 into estools:master Sep 5, 2012

1 check passed

default The Travis build passed
Details
@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Sep 5, 2012

Member

NOTE: In my environment (node version 0.8.8), npm test result is following.

Remember, that's using the SourceNodeMock constructor. I wouldn't expect that to be much slower. When actually generating source maps, we will see the biggest hit because we're flattening the entire tree structure into a string, usually just for a small portion of it.

Member

michaelficarra commented Sep 5, 2012

NOTE: In my environment (node version 0.8.8), npm test result is following.

Remember, that's using the SourceNodeMock constructor. I wouldn't expect that to be much slower. When actually generating source maps, we will see the biggest hit because we're flattening the entire tree structure into a string, usually just for a small portion of it.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Sep 5, 2012

Member

The mozilla/source-map pulls have been merged. Everything should be 100% operational.

Member

michaelficarra commented Sep 5, 2012

The mozilla/source-map pulls have been merged. Everything should be 100% operational.

@Constellation

This comment has been minimized.

Show comment
Hide comment
@Constellation

Constellation Sep 5, 2012

Member

The mozilla/source-map pulls have been merged. Everything should be 100% operational.

Great. After source-map package is published, we can generate source-map easily.

Member

Constellation commented Sep 5, 2012

The mozilla/source-map pulls have been merged. Everything should be 100% operational.

Great. After source-map package is published, we can generate source-map easily.

@fitzgen

This comment has been minimized.

Show comment
Hide comment
@fitzgen

fitzgen Sep 5, 2012

Contributor

I just pushed source-map@0.1.2 to npm.

Contributor

fitzgen commented Sep 5, 2012

I just pushed source-map@0.1.2 to npm.

@Constellation

This comment has been minimized.

Show comment
Hide comment
@Constellation

Constellation Sep 5, 2012

Member

@fitzgen
Awesome!
Because this is big change, I'm also planning to publish this as version 0.0.7 soon.

Member

Constellation commented Sep 5, 2012

@fitzgen
Awesome!
Because this is big change, I'm also planning to publish this as version 0.0.7 soon.

@fitzgen

This comment has been minimized.

Show comment
Hide comment
@fitzgen

fitzgen Sep 5, 2012

Contributor

I am very excited at the thought of many existing and new compilers which target JS using escodegen as a backend so that they can get source maps for free!

Contributor

fitzgen commented Sep 5, 2012

I am very excited at the thought of many existing and new compilers which target JS using escodegen as a backend so that they can get source maps for free!

Constellation added a commit that referenced this pull request Sep 5, 2012

@Constellation

This comment has been minimized.

Show comment
Hide comment
@Constellation

Constellation Sep 5, 2012

Member

I've just published version 0.0.7, shipping source map option!
Thanks all :-)

Member

Constellation commented Sep 5, 2012

I've just published version 0.0.7, shipping source map option!
Thanks all :-)

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Sep 5, 2012

Member

We should probably document the usage of the sourceMap option. Basically, if you provide a non-empty string value for options.sourceMap, escodegen will generate a source map, using the given string as the source map's source file name.

Member

michaelficarra commented Sep 5, 2012

We should probably document the usage of the sourceMap option. Basically, if you provide a non-empty string value for options.sourceMap, escodegen will generate a source map, using the given string as the source map's source file name.

@Constellation

This comment has been minimized.

Show comment
Hide comment
@Constellation

Constellation Sep 5, 2012

Member

We should probably document the usage of the sourceMap option.

Yes. I've just updated API wiki page.
https://github.com/Constellation/escodegen/wiki/API
Please feel free to update wiki page.

Member

Constellation commented Sep 5, 2012

We should probably document the usage of the sourceMap option.

Yes. I've just updated API wiki page.
https://github.com/Constellation/escodegen/wiki/API
Please feel free to update wiki page.

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