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

Provide a way to generate source map for javascript files #512

Open
jbrey opened this issue Jun 2, 2015 · 21 comments
Open

Provide a way to generate source map for javascript files #512

jbrey opened this issue Jun 2, 2015 · 21 comments

Comments

@jbrey
Copy link
Contributor

jbrey commented Jun 2, 2015

Hi,

It would be nice to provide an option to activate the generation of the source map for minified and/or aggregated files.
I saw that the CompilerOptions provides method to enable the generation of these files (http://javadoc.closure-compiler.googlecode.com/git/com/google/javascript/jscomp/CompilerOptions.html#setSourceMapOutputPath(java.lang.String).

Maybe I miss an existing option that would allow me to do that already ?

Cheers
JB

@barjo barjo added the planned label Jun 3, 2015
@barjo barjo self-assigned this Jun 3, 2015
@barjo
Copy link
Member

barjo commented Jun 3, 2015

Hi @jbrey,

As far as I know the generation of the map is already done by default when using the aggregation and or minification for coffee script. In the coffee script case, the map are created in the default target folder, along with the created js files.

We can add an option that allows for the creation of the source map files when creating the aggregates/minified files

@barjo
Copy link
Member

barjo commented Jun 3, 2015

It may be a bit tricky to do it properly.

@barjo barjo added in progress and removed planned labels Jun 3, 2015
barjo pushed a commit that referenced this issue Jun 3, 2015
@barjo barjo added the feature label Jun 3, 2015
@barjo
Copy link
Member

barjo commented Jun 3, 2015

@jbrey

I just push a new version that create the map file by default. It can be disabled with the configuration property googleClosureMap set to false.

Let me know if it's what you had in mind.

@jbrey
Copy link
Contributor Author

jbrey commented Jun 3, 2015

Thanks. I'll try ASAP!

@jbrey
Copy link
Contributor Author

jbrey commented Jun 3, 2015

I've tried.

The .map are generated with the expected file name, but...:

  • it does not seem to be activated for the default configuration (which generates a min file for every single js). Indeed, in my context, I have two executions: one for the minification & aggregation of the whole library and one execution without aggregation configuration for generating min files for every sources.
  • It may be a problem of the google closure library but when inspecting sources from chrome, the map file is not loaded and the sources still minified. It's probably because the min file does not have the reference to the map file.
    For example, here is what we can find at the end of the backbone-min.js file: //# sourceMappingURL=backbone-min.map

@barjo
Copy link
Member

barjo commented Jun 3, 2015

Thanks for the update,

I will investigate the first point. For the second point, I already append the //# sourceMappingURL= at the end of the created js file. I am not sure if it need a full path thought :-?

@jbrey
Copy link
Contributor Author

jbrey commented Jun 3, 2015

The url should be relative to the minified file to be found by the browser so not a full path.

@barjo
Copy link
Member

barjo commented Jun 3, 2015

It's awkward that it doesn't work out for you (it pass in the test). Could you check if you have such comment at the end of the created js file ?

@jbrey
Copy link
Contributor Author

jbrey commented Jun 3, 2015

In fact, I've just figured out that I have it in the aggregated file but not inside the aggregated and minified file.
I guess the comment is removed by the minifier.

@barjo
Copy link
Member

barjo commented Jun 3, 2015

Thanks for your help, I find out why it only work out for the aggregated file and not the minified one as well as your first issue. Basically, I do it only when there is an Aggregation and not when it's just a File 👎 I missed that. I will fix that tomorrow morning! (GMT +7)

@jbrey
Copy link
Contributor Author

jbrey commented Jun 3, 2015

Thank you for the quick answers and implementation :) .

@barjo
Copy link
Member

barjo commented Jun 4, 2015

@jbrey @cescoffier

That's a working version. I am doing som refactoring now and have some open questions.
When we create the source map files, should the the sources listed in the source map files be a full path? should it be relative to a particular path?

In this example, it is relative to the target folder. It can be a bit tricky to handle this source url, as it could depends if we are handling internals or externals assets.

{
"version":3,
"file":"street-min.js",
"lineCount":1,
"mappings":"AACAA,OAAAC,EAAA,CADUC,6BACV;",
"sources":["target/test-classes/fake-project/target/classes/assets/street.js"],
"names":["console","log","msg"]
}

Should we leave the option to choose the SourceMap output path from the configuration? In that case the sources path will be relatives to it. Or should we give a mapping, and replaces all source relative path by a full path. Or should we handle differently, depending if it's internal,externals or an agregation.

barjo pushed a commit that referenced this issue Jun 4, 2015
@jbrey
Copy link
Contributor Author

jbrey commented Jun 4, 2015

I think that all paths inside .map files should be relative to the file they refer to, without information on the build environment.

Moreover, when the package (webjar or zip or anything else) is deployed, the absolute path or path relative to target are not valid anymore.

IMHO, the paths to source files should be resolved from the output path of the .map file.

Example:
with the following directory structure (a bit strange but for the purpose of the example):

    a
        b
            b1.js
            b2.js
        c
            c1.js
            c2.js
        d
            e
                e1.js
                e2.js
            aggr.min.js
            aggr.min.js.map

I think the .map files should refer the following sources:

../b/b1.js, ../b/b2.js, ../c/c1.js, ../c/c2.js, ./e/e1.js, ./e/e2.js

Does it make sense ?

PS: I didn't really understood the comment on external assets because in order to have a consistent archive the aggregation (and/or minified) file and map file should refer only to the assets it contains, IMO.

Cheers
JB

@barjo
Copy link
Member

barjo commented Jun 4, 2015

Yes it makes sens.

I was planning to do that, but the way work the Source Map Output path is a bit tricky when you compile a list of file sharing the same options. There is no problem when we have a JavaScript configuration, since the ouput file is specified, we can set the source map output relative to the same path.

The problem is when handling the external and/or internal assets without an aggregation. We will compile a list of file, create the minified version and the map for each of them. So far so good, but they share the same configuration with the same source map output path (everything will be relative to it). I am not sure how I could handle that with the mapping :-?

I could also refactor to call a new compiler for each file with the proper source map output in that case (I was trying to avoid that). @cescoffier wdyt?

@barjo
Copy link
Member

barjo commented Jun 4, 2015

Since they share the same compiler, they all have the same source map file too.

@jbrey
Copy link
Contributor Author

jbrey commented Jun 4, 2015

ok, I understand the problem for maps without aggregation. I'm trying to do some resarch about it.

BTW, I think that having it working with the aggregations is already a good first step :)

@barjo
Copy link
Member

barjo commented Jun 4, 2015

Ok, so for now I will leave it for the aggregation only, with the expected behavior. And we will see later for the next step ;)

@barjo
Copy link
Member

barjo commented Jun 4, 2015

So, by default the SourceMap output will be the folder that contains the minified/aggregated file.

@jbrey
Copy link
Contributor Author

jbrey commented Jun 4, 2015

Fine for me. I'll try to have a look later too.

Thanks

@cescoffier
Copy link
Member

Could we close this one ?

@jbrey
Copy link
Contributor Author

jbrey commented Jun 15, 2015

I'm still having a look as a background task, as it generates the full absolute path of the workstation inside the map.

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

No branches or pull requests

3 participants