gulp-community / gulp-terser-js

A Terser-js plugin for Gulp
MIT License
12 stars 4 forks source link

Add content to sourcemaps #19

Closed BSFishy closed 4 years ago

BSFishy commented 4 years ago

I noticed that the content option is not passed to Terser. I've noticed that in my own projected it gives better results with less duplicate sources and things. Compiling from Typescript then passing that through Terser would give me weird things having do to with multiple sources that are the same file and a null source content. This shouldn't be a breaking change to my knowledge. It would be great to have this because currently my source maps are looking a little strange without it.

A-312 commented 4 years ago

Can you give me a short example of how we use your options ?

We should add a test: https://github.com/A-312/gulp-terser-js/tree/master/test and maybe a documentation about new option

BSFishy commented 4 years ago

I added a little bit of documentation in the readme. I looked at the test files, but wasn't able to understand the format very well. However, I don't think it is something that needs very extensive testing.

An example of what the change does is change the resulting source map from this:

{
  "version": 3,
  "sources": [
    "packages/storage/lib/index.js",
    "../lib/index.ts"
  ],
  "names": [
    "Object",
    "defineProperty",
    "exports",
    "value",
    "Storage",
    "default"
  ],
  "mappings": "AACAA,OAAOC,eAAeC,QAAS,aAAc,CAAEC,OAAO,ICEtD,MAAsBC,GAAtBF,QAAAE,QAAAA,EAIAF,QAAAG,QAAeD",
  "file": "index.js",
  "sourcesContent": [
    null,
    "/**\n *\n */\nexport abstract class Storage {\n\n}\n\nexport default Storage;\n"
  ]
}

To this:

{
  "version": 3,
  "sources": [
    "../lib/index.ts"
  ],
  "names": [
    "Storage",
    "exports",
    "default"
  ],
  "mappings": "uDAGA,MAAsBA,GAAtBC,QAAAD,QAAAA,EAIAC,QAAAC,QAAeF",
  "file": "index.js",
  "sourcesContent": [
    "/**\n *\n */\nexport abstract class Storage {\n\n}\n\nexport default Storage;\n"
  ]
}

The second one is a much better representation, because the code this is from is written in Typescript, and doesn't use some of the properties like Object or defineProperty. So, instead of Terser making a source map from the resulting Javascript, it is able to use the information provided from Typescript.

To reiterate: this is a drop-in replacement. No code needs to be changed in client code and everything should be compatible. The only difference is that resulting source maps from compiled Javascript should look a little nicer.

BSFishy commented 4 years ago

(Also, I think it would be better to use a version string like ^4.6.0 for Terser so that newer versions of Terser can be used without worrying about breaking changes)

A-312 commented 4 years ago

Can you fix: https://travis-ci.org/github/A-312/gulp-terser-js/builds/678220065 ?

BSFishy commented 4 years ago

So, here is where I am at right now. I fixed a test case (I think it was already broken, but it was as simple as fixing the expected output). However, the actual error doesn't seem to have as simple of a fix. It seems that if the source map doesn't already contain mapping information and such, the output source map is pretty much completely empty. This is why it was working for my personal project; I was compiling from Typescript, which gave proper mapping information. I want to make it clear that I don't exactly know all of the conditions that must be met to cause the error.

I have already made an issue on Terser (terser/terser#660) to discuss the issue on that end. However there is still some changes that can be made with this project. Namely, how the source maps options are handled. As of now, if the file has a sourceMap property, the existing source map options will be discarded. This means that if the end user were to specify their own source map options, they wouldn't be used. I think that at the very least, the Terser source map options should be supported. Personally, I have a few ideas on how this could be implemented, but I wanted to converse here about how it should be done, as to not stray from how the plugin is intended to function.

First of all, if source maps are detected, it could first check if sourceMap already exists. If not, create a new object. Secondly, perhaps the content option could be true if the user wants the existing source map to be used, otherwise use the provided content option, if it exists.

I.e., if the user wants to use file.sourceMap as the value for opts.sourceMap.content, their options might look like this:

.pipe(terser({
  sourceMap: {
    content: true
  }
}))
BSFishy commented 4 years ago

@A-312 I fixed the tests. I made the sourceMap option overridable. I also made content able to be set to true which will use the file's existing source map. An example would be like this:

gulp.src('asset/js/*.js')
  .pipe(sourcemaps.init({ loadMaps: true }))
  .pipe(terser({
     sourceMap: {
       content: true
     }
  }))
  .pipe(sourcemaps.write())
  .pipe(gulp.dest('dist'))

Is this okay?

A-312 commented 4 years ago

I will try to review and publish the new version before Monday afternoon.

A-312 commented 4 years ago

My week schedule was changed, I don't forgot!

A-312 commented 4 years ago

@BSFishy Thanks for your contribution, 5.2.0 is online (with caret version for dependencies)