-
Notifications
You must be signed in to change notification settings - Fork 21
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
added support for sourcemap #9
Conversation
Hey, this sounds cool. Any reasons against the merge (apart from the unresolved conflict)? |
Yes, can we get this merged? What needs to be done to move it forward? |
FYI, there's some relevant discussion happening in joliss/broccoli-uglify-js#8. |
turns out coffeescript support embedded sourcemap
Fixed conflict 😃 |
literate: coffeeScript.helpers.isLiterate(srcFile), | ||
sourceMap: !!this.sourceMap, | ||
|
||
sourceFiles: [ srcFile ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Given an input file dir/foo.coffee
that gets compiled into dir/foo.js
, this sourceFiles
option will result in the following source map output:
"sources": [
"dir/foo.coffee"
],
But "sources" are URLs relative to dir/foo.js
. Thus if you load /dir/foo.js
with a <script>
tag, Chrome inspector shows the CoffeeScript source as dir/dir/foo.coffee
in its sources panel.
Using sourceFiles: [ path.basename(srcFile) ]
here is a possible workaround, but it seems that this should really be fixed on CoffeeScript's end: jashkenas/coffeescript#3296
I'd be OK using the basename
workaround though if you think that's reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't path.basename
just going to flatten all the files? If there are 2 files of the same name in different directories, they will be displayed as the same file.
(Also tracking jashkenas/coffeescript#3678 in case the CoffeeScript compiler gains native support for data URLs at some point.) |
Great! Thanks, I'll try it out. |
+1 |
Hi! |
+1 |
+5 |
@joliss Unless you think this is going to break anything, I think it's better to merge this. I think inaction in this case is actually doing more harm than good. If this implementation of source map is not good enough, we can always have other contributors to fix it. |
It seems @joliss was waiting for the comments left in commits to be fixed, but then abandoned the project for those last months? |
+1 |
Any chance of releasing this? |
It turns out CoffeeScript support embedded source map.
So I just added a switch in options to additionally embed the CoffeeScript source into the output. This should resolve #1