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

Add content to sourcemaps #19

Merged
merged 3 commits into from
Apr 30, 2020
Merged

Add content to sourcemaps #19

merged 3 commits into from
Apr 30, 2020

Conversation

BSFishy
Copy link
Contributor

@BSFishy BSFishy commented Apr 22, 2020

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
Copy link
Member

A-312 commented Apr 22, 2020

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
Copy link
Contributor Author

BSFishy commented Apr 22, 2020

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
Copy link
Contributor Author

BSFishy commented Apr 22, 2020

(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
Copy link
Member

A-312 commented Apr 22, 2020

@BSFishy
Copy link
Contributor Author

BSFishy commented Apr 22, 2020

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
  }
}))

@A-312 A-312 self-requested a review April 22, 2020 21:07
Copy link
Member

@A-312 A-312 left a comment

Choose a reason for hiding this comment

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

(My test was fine)

I don't know why but travis.org refuse to link the CI on this PR.

@BSFishy
Copy link
Contributor Author

BSFishy commented Apr 25, 2020

@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 A-312 self-requested a review April 25, 2020 01:23
@A-312
Copy link
Member

A-312 commented Apr 25, 2020

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

@A-312
Copy link
Member

A-312 commented Apr 28, 2020

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

@A-312 A-312 merged commit 4d33d9a into gulp-community:master Apr 30, 2020
@A-312
Copy link
Member

A-312 commented Apr 30, 2020

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

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

Successfully merging this pull request may close these issues.

None yet

2 participants