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

Supporting input sourceMap comment parsing #297

Closed
guybedford opened this Issue Sep 2, 2015 · 23 comments

Comments

Projects
None yet
9 participants
@guybedford
Member

guybedford commented Sep 2, 2015

Parsing out sourceMap from input sources and supporting them for the full bundle build. This should go along with systemjs/systemjs#638.

load.metadata.sourceMap should obviously still take preference, with these comments effectively filling that meta in SystemJS with the detection enabled.

Similarly, we should strip the comments (along with other unnecessary meta) when doing concatenation.

@Rush

This comment has been minimized.

Show comment
Hide comment
@Rush

Rush Sep 5, 2015

I gave it a shot ... I modified trace.js and its source loading step to:

    return Promise.resolve(loader.fetch({ name: normalized, metadata: load.metadata, address: load.address }))
      .then(function(source) {

        var sourceMappingURL = require('source-map-url');
        var dataUriToBuffer = require('data-uri-to-buffer');
        var sourceMap = sourceMappingURL.getFrom(source);
        if(sourceMap) {
          source = sourceMappingURL.removeFrom(source);
          if(!load.metadata.sourceMap) {
            load.metadata.sourceMap = dataUriToBuffer(sourceMap).toString('utf8');
          }
        }    
        load.originalSource = source;
        return loader.translate({ name: normalized, metadata: load.metadata, address: load.address, source: source });
      })

I can see the final source map's size being affected a little bit and I see that AMD module is reading this source map ... but still it doesn't work. Sources in Chrome show the source after the initial transformation with the inline source map. Any tips? Is it the right place I am tinkering with?

Rush commented Sep 5, 2015

I gave it a shot ... I modified trace.js and its source loading step to:

    return Promise.resolve(loader.fetch({ name: normalized, metadata: load.metadata, address: load.address }))
      .then(function(source) {

        var sourceMappingURL = require('source-map-url');
        var dataUriToBuffer = require('data-uri-to-buffer');
        var sourceMap = sourceMappingURL.getFrom(source);
        if(sourceMap) {
          source = sourceMappingURL.removeFrom(source);
          if(!load.metadata.sourceMap) {
            load.metadata.sourceMap = dataUriToBuffer(sourceMap).toString('utf8');
          }
        }    
        load.originalSource = source;
        return loader.translate({ name: normalized, metadata: load.metadata, address: load.address, source: source });
      })

I can see the final source map's size being affected a little bit and I see that AMD module is reading this source map ... but still it doesn't work. Sources in Chrome show the source after the initial transformation with the inline source map. Any tips? Is it the right place I am tinkering with?

@guybedford

This comment has been minimized.

Show comment
Hide comment
@guybedford

guybedford Sep 8, 2015

Member

@Rush thanks for looking into this, this looks great, and the right place to be doing it as well. I think this is just about debugging at this point. I'd suggest not setting originalSource though as that is for internal use with the compilers. Are you running minification here at all?

Member

guybedford commented Sep 8, 2015

@Rush thanks for looking into this, this looks great, and the right place to be doing it as well. I think this is just about debugging at this point. I'd suggest not setting originalSource though as that is for internal use with the compilers. Are you running minification here at all?

@Haemoglobin

This comment has been minimized.

Show comment
Hide comment
@Haemoglobin

Haemoglobin Sep 15, 2015

Hi,

Just wondering if this issue is related to a problem I'm trying to figure out.

I am wanting to go from typescript files with es6 module syntax and transpile those to systemjs format including embedded sourcemaps back to ts files (so far so good, tsc supports this ok).

Now I want to bundle these into one file by running systemjs-builder on those transpiled files (with sourcemaps turned on). The final sourcemap for the bundle however ends up pointing to transpiled ts source as opposed original ts files. Would this feature allow for this? Or is it possible already?

Thanks!
Hamish

Haemoglobin commented Sep 15, 2015

Hi,

Just wondering if this issue is related to a problem I'm trying to figure out.

I am wanting to go from typescript files with es6 module syntax and transpile those to systemjs format including embedded sourcemaps back to ts files (so far so good, tsc supports this ok).

Now I want to bundle these into one file by running systemjs-builder on those transpiled files (with sourcemaps turned on). The final sourcemap for the bundle however ends up pointing to transpiled ts source as opposed original ts files. Would this feature allow for this? Or is it possible already?

Thanks!
Hamish

@guybedford

This comment has been minimized.

Show comment
Hide comment
@guybedford

guybedford Sep 17, 2015

Member

@Haemoglobin yes this sounds like the same issue.

Member

guybedford commented Sep 17, 2015

@Haemoglobin yes this sounds like the same issue.

@drieks

This comment has been minimized.

Show comment
Hide comment
@drieks

drieks Oct 1, 2015

Hello, we have the same problem here.
Has anybody new informations about this problem?

drieks commented Oct 1, 2015

Hello, we have the same problem here.
Has anybody new informations about this problem?

@stasberkov

This comment has been minimized.

Show comment
Hide comment
@stasberkov

stasberkov Oct 6, 2015

+1
We need this feature to start using systemjs-builder. Otherwise developers are complaining.

stasberkov commented Oct 6, 2015

+1
We need this feature to start using systemjs-builder. Otherwise developers are complaining.

@laurentdesmet

This comment has been minimized.

Show comment
Hide comment

laurentdesmet commented Oct 16, 2015

+1

@Rush

This comment has been minimized.

Show comment
Hide comment
@Rush

Rush Oct 19, 2015

Unfortunately I didn't have time to work more on this issue ... but if anybody has time you can start off by using the code I pasted above.

Rush commented Oct 19, 2015

Unfortunately I didn't have time to work more on this issue ... but if anybody has time you can start off by using the code I pasted above.

@jpeters5392

This comment has been minimized.

Show comment
Hide comment
@jpeters5392

jpeters5392 Nov 17, 2015

@Rush I was able to get this working locally with a slight tweak of your approach. There have been quite a few commits to this file since Sept 4 but my version is based off of the most recent code.

return loader.fetch({ name: normalized, metadata: load.metadata, address: address })
        .then(function(source) {
          if (typeof source != 'string')
            throw new TypeError('Loader fetch hook did not return a source string');

          // this step will look for a comment containing the inline source map
      var sourceMappingURL = require('source-map-url');
      var dataUriToBuffer = require('data-uri-to-buffer');
      var sourceMap = sourceMappingURL.getFrom(source);
          // TODO: the following only supports a data URI encoded source map but sourceMappingUrl can also be a URL to an external source map file.  Additional work is needed for external source maps
      if(sourceMap && sourceMap.startsWith('data:')) {
        source = sourceMappingURL.removeFrom(source);
        if(!load.metadata.sourceMap) {                       
            load.metadata.sourceMap = dataUriToBuffer(sourceMap).toString('utf8');
        }
      } 

      originalSource = source;
          curHook = 'translate';

          // default loader fetch hook will set load.metadata.timestamp
          if (load.metadata.timestamp) {
            load.timestamp = load.metadata.timestamp;
            load.metadata.timestamp = undefined;
          }

          return loader.translate({ name: normalized, metadata: load.metadata, address: address, source: source });
        })

jpeters5392 commented Nov 17, 2015

@Rush I was able to get this working locally with a slight tweak of your approach. There have been quite a few commits to this file since Sept 4 but my version is based off of the most recent code.

return loader.fetch({ name: normalized, metadata: load.metadata, address: address })
        .then(function(source) {
          if (typeof source != 'string')
            throw new TypeError('Loader fetch hook did not return a source string');

          // this step will look for a comment containing the inline source map
      var sourceMappingURL = require('source-map-url');
      var dataUriToBuffer = require('data-uri-to-buffer');
      var sourceMap = sourceMappingURL.getFrom(source);
          // TODO: the following only supports a data URI encoded source map but sourceMappingUrl can also be a URL to an external source map file.  Additional work is needed for external source maps
      if(sourceMap && sourceMap.startsWith('data:')) {
        source = sourceMappingURL.removeFrom(source);
        if(!load.metadata.sourceMap) {                       
            load.metadata.sourceMap = dataUriToBuffer(sourceMap).toString('utf8');
        }
      } 

      originalSource = source;
          curHook = 'translate';

          // default loader fetch hook will set load.metadata.timestamp
          if (load.metadata.timestamp) {
            load.timestamp = load.metadata.timestamp;
            load.metadata.timestamp = undefined;
          }

          return loader.translate({ name: normalized, metadata: load.metadata, address: address, source: source });
        })
@Rush

This comment has been minimized.

Show comment
Hide comment
@Rush

Rush Nov 17, 2015

@jpeters5392 sounds great, will give it a shot later. Actually this is the issue that has been stopping me from using ES6 ... so looking forward to it!

Rush commented Nov 17, 2015

@jpeters5392 sounds great, will give it a shot later. Actually this is the issue that has been stopping me from using ES6 ... so looking forward to it!

@Rush

This comment has been minimized.

Show comment
Hide comment
@Rush

Rush Dec 25, 2015

@jpeters5392 thanks, managed to test your code and it seems promising. Sources of the files do appear in the Chrome Sources tab, however those sources do not appear as the original ones, what Chrome shows for me is this source = sourceMappingURL.removeFrom(source), so basically Chrome shows transpiled ES5 sources for me and not ES2015. Maybe we need to apply the source map here to get the oiriginal code instead of simply removing it from the string?

Rush commented Dec 25, 2015

@jpeters5392 thanks, managed to test your code and it seems promising. Sources of the files do appear in the Chrome Sources tab, however those sources do not appear as the original ones, what Chrome shows for me is this source = sourceMappingURL.removeFrom(source), so basically Chrome shows transpiled ES5 sources for me and not ES2015. Maybe we need to apply the source map here to get the oiriginal code instead of simply removing it from the string?

@doxavore

This comment has been minimized.

Show comment
Hide comment
@doxavore

doxavore Dec 28, 2015

@jpeters5392 I'm running into the same issue as @Rush when attempting to apply your patch. I don't suppose you have an example of the ES6/TypeScript-to-ES5 pipeline you're using? Perhaps there's something elsewhere in the stack that is helping your use case.

doxavore commented Dec 28, 2015

@jpeters5392 I'm running into the same issue as @Rush when attempting to apply your patch. I don't suppose you have an example of the ES6/TypeScript-to-ES5 pipeline you're using? Perhaps there's something elsewhere in the stack that is helping your use case.

@jpeters5392

This comment has been minimized.

Show comment
Hide comment
@jpeters5392

jpeters5392 Dec 28, 2015

@Rush @doxavore Unfortunately my team at work decided to change our build pipeline so I do not have this running locally anymore. If I remember correctly we were using Babel 5.x to transpile and then we were using SystemJS to bundle the transpiled code. I think we were using the minify option with SystemJS.

jpeters5392 commented Dec 28, 2015

@Rush @doxavore Unfortunately my team at work decided to change our build pipeline so I do not have this running locally anymore. If I remember correctly we were using Babel 5.x to transpile and then we were using SystemJS to bundle the transpiled code. I think we were using the minify option with SystemJS.

@asapach

This comment has been minimized.

Show comment
Hide comment
@asapach

asapach Mar 19, 2016

Member

I've spent some time debugging and digging through the code and I think I got it working: #530
One of the problems I've noticed is that some of the resulting source maps get a bit garbled after the transformation (e.g. sometimes lines don't match) - I blame Traceur.
Could use some help writing the unit tests to verify that it works and that it doesn't break anything.

Member

asapach commented Mar 19, 2016

I've spent some time debugging and digging through the code and I think I got it working: #530
One of the problems I've noticed is that some of the resulting source maps get a bit garbled after the transformation (e.g. sometimes lines don't match) - I blame Traceur.
Could use some help writing the unit tests to verify that it works and that it doesn't break anything.

@Rush

This comment has been minimized.

Show comment
Hide comment
@Rush

Rush Mar 20, 2016

@asapach Idea, to fix bad newline mappings try calling uglify with the following:

            beautify: true,
            indent_level: 0,
            semicolons: false,
            space_colon: false

Works for another project of mine which has to call uglify manually. This will uglify everything besides the newlines. It increases the size slightly so it's not a perfect solution ...

Rush commented Mar 20, 2016

@asapach Idea, to fix bad newline mappings try calling uglify with the following:

            beautify: true,
            indent_level: 0,
            semicolons: false,
            space_colon: false

Works for another project of mine which has to call uglify manually. This will uglify everything besides the newlines. It increases the size slightly so it's not a perfect solution ...

@asapach

This comment has been minimized.

Show comment
Hide comment
@asapach

asapach Mar 20, 2016

Member

I've been testing it mostly without minification, so uglify is not to blame.
Looks like additional work needs to be done to make it work with minify: true

Member

asapach commented Mar 20, 2016

I've been testing it mostly without minification, so uglify is not to blame.
Looks like additional work needs to be done to make it work with minify: true

@asapach

This comment has been minimized.

Show comment
Hide comment
@asapach

asapach Apr 25, 2016

Member

Here's my second attempt: #568. This time I've added some unit tests to verify.

Member

asapach commented Apr 25, 2016

Here's my second attempt: #568. This time I've added some unit tests to verify.

@guybedford

This comment has been minimized.

Show comment
Hide comment
@guybedford

guybedford May 18, 2016

Member

Supported in the coming release thanks to @asapach.

Member

guybedford commented May 18, 2016

Supported in the coming release thanks to @asapach.

@guybedford

This comment has been minimized.

Show comment
Hide comment
@guybedford

guybedford May 19, 2016

Member

Released in 0.15.17.

Member

guybedford commented May 19, 2016

Released in 0.15.17.

@Rush

This comment has been minimized.

Show comment
Hide comment
@Rush

Rush May 19, 2016

Great job @asapach, @guybedford

Looks like additional work needs to be done to make it work with minify: true

Out of curiosity, what was the additional work here?

Rush commented May 19, 2016

Great job @asapach, @guybedford

Looks like additional work needs to be done to make it work with minify: true

Out of curiosity, what was the additional work here?

@asapach

This comment has been minimized.

Show comment
Hide comment
@asapach

asapach May 19, 2016

Member

I needed to run more tests on real-life projects to see how different options affected the output. I've also fixed the source maps tests (which were previously disabled) and added my own.
The two issues I ran into:

  • gulp-sourcemaps by default produces unusable source maps - you need to pass the correct sourceRoot option to make it work
  • The source maps produced by Builder sometimes have incorrect offsets, which is looks like a bug in Traceur.
Member

asapach commented May 19, 2016

I needed to run more tests on real-life projects to see how different options affected the output. I've also fixed the source maps tests (which were previously disabled) and added my own.
The two issues I ran into:

  • gulp-sourcemaps by default produces unusable source maps - you need to pass the correct sourceRoot option to make it work
  • The source maps produced by Builder sometimes have incorrect offsets, which is looks like a bug in Traceur.
@Rush

This comment has been minimized.

Show comment
Hide comment
@Rush

Rush May 19, 2016

you need to pass the correct sourceRoot option to make it work

What's a correct sourceRoot?

The source maps produced by Builder sometimes have incorrect offsets, which is looks like a bug in Traceur.

This is probably the thing I wrote #297 (comment) - this made offsets work for me in other circumstances.

Rush commented May 19, 2016

you need to pass the correct sourceRoot option to make it work

What's a correct sourceRoot?

The source maps produced by Builder sometimes have incorrect offsets, which is looks like a bug in Traceur.

This is probably the thing I wrote #297 (comment) - this made offsets work for me in other circumstances.

@asapach

This comment has been minimized.

Show comment
Hide comment
@asapach

asapach May 19, 2016

Member

The sourceRoot depends on your particular setup. The idea is that file in the source map must be a valid relative path from the baseURL, and sourceRoot + sources should be a valid relative path from the file to the source file. I think there's a plugin that addresses this: https://www.npmjs.com/package/gulp-relative-sourcemaps-source

Member

asapach commented May 19, 2016

The sourceRoot depends on your particular setup. The idea is that file in the source map must be a valid relative path from the baseURL, and sourceRoot + sources should be a valid relative path from the file to the source file. I think there's a plugin that addresses this: https://www.npmjs.com/package/gulp-relative-sourcemaps-source

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