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

Sourcemaps #346

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Sourcemaps #346

wants to merge 14 commits into from

Conversation

aehlke
Copy link

@aehlke aehlke commented Jun 9, 2014

This isn't ready to merge as is, but this fork adds the groundwork needed for generating source maps for JS/CSS. I haven't included my plugin for Closure, but you can see the interface in the changes below (which are pretty minimal). Sharing this to get your input, maybe this can be adapted to some way you prefer.

@aehlke
Copy link
Author

aehlke commented Jun 18, 2014

Any thoughts on this? Would be interested to hear if you think this approach is good, then I can invest more time in getting these architecture changes back upstream. Thanks.

@cyberdelia
Copy link
Member

I'm not fond of passing get_js around.
I also would love to see if it works with for coffee-script, sass, etc...

@aehlke
Copy link
Author

aehlke commented Jul 4, 2014

@cyberdelia Good catch, it turned out passing in get_js wasn't necessary since I split the compressors into two possible methods: compress_foo (which receives the file contents as before) and compress_foo_with_source_map (which doesn't need the file contents - AFAIK it's a hard requirement of any source compressor with source mapping to know the individual files, not just their concatenated contents). I'll refactor once I have some time.

Haven't looked at adding this support to compilers yet.

@aehlke
Copy link
Author

aehlke commented Jul 4, 2014

Another issue I just realized: putting the source map's filename in the source makes it tricky to use Django's staticfiles hashed filenames. I see one possible solution: add post-processing to the staticfiles storage classes to rewrite source map filenames in the source files. This sucks, but I'm not sure how to avoid this - thoughts?

@aehlke
Copy link
Author

aehlke commented Jul 4, 2014

Oh d'oh we can just extend the HashedFilesMixin's patterns for URL rewriting.

@danxshap
Copy link

danxshap commented Jul 6, 2014

@aehlke would you mind bringing me up to speed on what you're trying to do here / how you're trying to do it?

I see you're talking about hashed filenames above - does that mean you're trying to get sourcemaps working in a production environment? I've only just discovered the existence of sourcemaps, but my understanding is that they're mostly useful in development when you're trying to look at compiled (not compressed) output in Chrome DevTools or similar.

In my situation, I'd like to use them with Sass as described here and don't care about using them in production - just need to be able to see my partials in DevTools instead of the compiled CSS.

This actually mostly works out of the box just by adding this setting: PIPELINE_SASS_ARGUMENTS = '--sourcemap'. The sourcemap gets generated, Chrome downloads it fine, and it shows me the Sass partials and accurate line numbers mapping generated CSS back to Sass - amazing already!

The part that doesn't completely work is if I want to have Chrome show me the partial's source (& let me live-edit it if I'm using the Workspaces feature). It works (coincidentally) for partials that are in the same app static directory as the sourcemap file, but for other app static directories it doesn't work because Sass has no clue about Django's static URL conventions. See what I mean here: http://d.pr/i/sqX8

In the sources array in the sourcemap, I get something like this:
../../../../lessons/static/lessons/course/scss/_paid_unit_list.scss

When what I really need is this:
/static/lessons/course/scss/_paid_unit_list.scss

So, based on my very limited understanding/experience with sourcemaps, all I need to do to get this working perfectly in development is rewrite some of the URLs in the sources array accordingly and make sure STATIC_URL is the prefix.

This is the approach that these guys are using: sindresorhus/gulp-ruby-sass#68

In the case of django-pipeline, couldn't that just be a new setting that holds a regex to replace with STATIC_URL, which gets applied to every item in the sources array?

Sorry for long comment. What do you think?

@aehlke
Copy link
Author

aehlke commented Jul 7, 2014

@danxshap I haven't looked into SASS yet (and FYI your PIPELINE_SASS_ARGUMENTS solution only works as long as you don't concatenate files, which is reasonable for local dev) but I'm guessing your issue may be solved by extending the work I intend to do on adding URL rewriting for sourcemap file locations. I'm using Django 1.7's HashedFilesMixin (back-ported to 1.6) which has a patterns property for rewriting URLs: https://github.com/django/django/blob/1.7c1/django/contrib/staticfiles/storage.py#L53 This just needs to be extended to rewrite sourceMappingURL=foo.bar.map using staticfiles for the new URL.

@aehlke
Copy link
Author

aehlke commented Jul 7, 2014

@danxshap The reason I care about using sourcemaps in production though is that we'd like to use Sentry (https://www.getsentry.com/) for our JS and have meaningful/readable tracebacks in the errors we log.

@danxshap
Copy link

danxshap commented Jul 7, 2014

@aehlke got it. I don't know how sourcemaps work for JS files, but if they can map an exception in a concatenated, compiled (coffeescript?), and minified file to the specific line number in the individual, uncompiled & un-minified file - that is amazing!

Looking forward to trying out what you come up with.

In the mean time, I've decided to use this solution for Sass in my own project which is working great in development (in case anyone stumbles upon this and has the same issue): https://gist.github.com/danxshap/193a74609291150c216d

@aehlke
Copy link
Author

aehlke commented Jul 8, 2014

@danxshap Indeed, Google's Closure compiler for JS can take a list of JS files and output a single concatenated/minified file and a source map to go with it. :)

The solution you linked looks like it'll work, but it could extend the built-in way Django has for rewriting URLs in collectstatic post-processing. I implemented this in my own codebase yesterday but unfortunately it's blocked on a bug in Django that I've made a PR for: django/django#2895 It's easy to back-port this bugfix but it's a big chunk of code to bundle with django-pipeline...

@aehlke
Copy link
Author

aehlke commented Jul 8, 2014

@cyberdelia I've removed the passing-around of get_js and fixed an issue where staticfiles wasn't aware of the generated .map files (through refactoring pack to yield output filenames rather than return a single one). Unfortunately to extend HashedFilesMixin.patterns / CachedFilesMixin.patterns to rewrite sourceMappingURL in a post-processing step, we're going to have to wait for Django to accept my patch (django/django#2895) or back-port my patched post_process into pipeline (a lot of code to duplicate...).

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.

4 participants