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 sourcemap support #490

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@fdintino
Copy link
Member

fdintino commented Aug 17, 2015

This pull request is based on #346, but somewhat cleaned up (and it looks like that PR might be abandoned).

@fdintino fdintino force-pushed the theatlantic:sourcemaps branch from d6aeafe to 02745e4 Oct 24, 2015

@fdintino fdintino force-pushed the theatlantic:sourcemaps branch from 02745e4 to ec660fe Jan 4, 2016

@fdintino

This comment has been minimized.

Copy link
Member

fdintino commented Jan 6, 2016

What's the status of this PR? Any chance it could be merged in at some point?

@jezdez

This comment has been minimized.

Copy link
Member

jezdez commented Jan 19, 2016

@fdintino This needs tests and docs at least. Are there other compressor supporting source maps that could be added at the same time?

@fdintino

This comment has been minimized.

Copy link
Member

fdintino commented Jan 20, 2016

I've also written support for sourcemaps for uglifyjs and cssclean.

I agree on the tests, though I wasn't sure what the best way to go about that is. My first thought was to get node and java packages installed with the .travis.yml and bootstrap the different test cases with package.json files installed into temp directories (there is even an npm package to get closure.jar, so I could even use it for that). I would of course check if node or java is in the PATH and if not raise a SkipTest

@aehlke

This comment has been minimized.

Copy link

aehlke commented Jan 25, 2016

Thanks for this. I haven't had capacity for working more on my old PR - appreciate you picking this up!

source_abs_path, staticfiles_storage.base_location)
source_url = staticfiles_storage.url(source_rel_path)
source_map_data['sources'][i] = relurl(source_url, output_url)
source_map = json.dumps(source_map_data, indent="\t")

This comment has been minimized.

@Spacerat

Spacerat Feb 4, 2016

Contributor

Python 2.7's json.dumps doesn't allow strings as arguments for "indent" so I had to change this to indent=4 locally

This comment has been minimized.

@fdintino

fdintino Feb 4, 2016

Member

Thanks. We use python 2.7 so I'm not sure how this initially escaped my notice, but I discovered the same thing and just haven't pushed out the change (I'm still waiting for my unit test branch to get merged in before I rebase this pull request off of it).

@Spacerat

This comment has been minimized.

Copy link
Contributor

Spacerat commented Feb 9, 2016

Just a thought, do you think it's enough to add source map support at the compression stage but not the compilation stage? Without source maps for compilers, what do you do about coffeescript or ES6/jsx files?

@fdintino

This comment has been minimized.

Copy link
Member

fdintino commented Feb 10, 2016

Provided that the compiler stage adds the sourceMappingURL comment at the bottom of the file, then the cleancss and closure compressors will merge the original input source maps into the final one. We use the sass compiler with sassc and the --sourcemap flag1 and it works great with the cleancss compressor (if you want, inspect an element in Chrome on http://www.theatlantic.com/ to see it at work). I haven't tested that it works with closure (though I plan on writing unit tests for that once #531 is merged) but in theory it should, as I explicitly parse and pass along the input source maps in the closure arguments. UglifyJS only supports a single input source map via the cli for now but the support already exists in the API for multiple input source maps so you could customize the uglifyjs bin script to add this feature for yourself until the cli ticket is fixed. Or use the closure compressor, though it's about 10 times slower than uglify. None of the other bundled compressors support input source maps, so it would have to be between the above tools if you want this feature. The only other tool I'm aware of that can minify and concatenate input source maps is gulp. And I did write a gulp compressor before we switched to cleancss. But it's a terrible mess of dynamically building a Gulpfile.js in a temp directory and executing it with that cwd.

1 Annoyingly, sassc (i.e. libsass), node-sass, and ruby sass all have different command-line flags for enabling source map output

@fdintino

This comment has been minimized.

Copy link
Member

fdintino commented Feb 10, 2016

As you can tell, I've thought a lot about this :-)

So the short answer I suppose is that you can add the appropriate flags in your %(COMPILER)s_ARGUMENTS pipeline setting to enable sourcemaps for the time being. I think it would make sense for the OUTPUT_SOURCEMAPS setting to automatically add these flags to compilers that support source maps when True. But to be honest, given that it works well enough for our specific purposes, I'm not very motivated to improve upon this pull request until I get a sense that the maintainers (and I suppose specifically @cyberdelia) are ever going to merge them or even give feedback.

@fdintino fdintino force-pushed the theatlantic:sourcemaps branch from 6fc21a5 to 7c1bb9f Feb 23, 2016

@fdintino

This comment has been minimized.

Copy link
Member

fdintino commented Feb 23, 2016

This doesn't yet include the unit tests for source maps, but it's freshly rebased off of master (resolving the conflicts) and a few minor fixes are included, in case anyone wants to use this branch while I add tests and documentation.

@@ -14,9 +14,15 @@ def match_file(self, filename):

def compile_file(self, infile, outfile, outdated=False, force=False):
# Pipe to file rather than provide outfile arg due to a bug in lessc

This comment has been minimized.

@davidt

davidt Mar 3, 2016

Contributor

This comment is now obsolete.

@tobz

This comment has been minimized.

Copy link

tobz commented Jun 17, 2016

@cyberdelia @davidt

Is there any hope of ever merging this? It seems like a lot of the more meaningful, architectural PRs are sort of just being ignored out-of-hand, and I've yet to see any explanation as to why.

@fdintino

This comment has been minimized.

Copy link
Member

fdintino commented Jun 17, 2016

@tobz This PR isn't being ignored, it's on me actually to finish it. I have some work I haven't pushed out yet that allows for sourcemaps for the post-processors that support it, but getting those sourcemaps to concatenate in the compressor step has proved challenging, specifically for javascript. uglifyjs does not support multiple input sourcemaps, nor does closure compiler. One option is to concatenate the sourcemaps in python (the source map spec is not all that complicated) and then pass a single input sourcemap to uglifyjs, but when I attempted that the performance isn't great. I'll work a bit more on it and perhaps leave multilevel sourcemap support for a future pull request.

@Spacerat

This comment has been minimized.

Copy link
Contributor

Spacerat commented Jun 27, 2016

@fdintino Adding my two cents on what worked for me: I ended up going with the 'concatenate sourcemaps then feed them to uglifyjs' option, and I used your source-map-concat-cli package to do the compression. Then I wrote a custom compressor to tie it all together. I never cared too much to test the performance since everything is only compiled once for Production anyway, although it seems to run fast enough in development.

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