Use inline source maps when transpiling coffee-script #12713
Conversation
CI has trouble retranspiling source map references to files, so I don't know if |
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.
I approve those changes
@@ -99,7 +99,7 @@ function writeCachedJavascript (relativeCachePath, code) { | |||
|
|||
function addSourceURL (jsCode, filePath) { |
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.
Every one of our compilers already adds the source map and does the file:// work on Windows. I suspect this entire function can be removed.
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.
Verified, yep, it can be removed.
There is still trouble with getting devtools to load .coffee files from asar. It will pretty much break as soon as the person building deletes their build directory. |
Yeah, to make this work we would probably need to modify Electron to make the dev tools aware of I think that instead, we should just remove the source map URLs from the transpiled files that we ship. Eventually, we will stop using CoffeeScript. Though we may still use Babel to transpile our source files, the output will be pretty easy to read compared to the output of the CoffeeScript compiler. |
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.
@Ingramz are you interested in submitting a different PR to simply remove the source maps from the transpiled files that we ship?
I believe we'd need to change these files:
https://github.com/atom/atom/blob/master/script/lib/transpile-babel-paths.js
https://github.com/atom/atom/blob/master/script/lib/transpile-coffee-script-paths.js
As for this PR, in my opinion it is not worth including the un-transpiled version of every file in the app, especially since there would be some complexity in getting them to display correctly in the dev tools.
I'm not keen on losing the source maps entirely as they're pretty essential for debugging in our current Coffeescript world and even with Babel without them setting breakpoints etc. will be wrong. One compromise might be to only include them in local dev builds and not include them in production/release versions. |
I think this is the route we should take, at least until we can find a better solution (which will likely require changes in Electron as @maxbrunsfeld has mentioned). |
They already don't work, right? AFAIK, the only time you'd see coffee-script files are when the coffee-script has been compiled dynamically, e.g. non-bundled packages, or Atom's source code when running in dev or test mode. |
Some of them do work now but not all it would seem. I need to investigate. On Thu, Sep 22, 2016, 5:50 PM Max Brunsfeld notifications@github.com
|
I've found that commenting out the unlink of coffeescript files, one of the changes in this PR, has made debugging e.g. |
👍 I just reproduced both of these behaviors and am understanding this a little better now. So in our new build scripts, coffee-script files are compiled with source maps, but the source map paths point to a temporary location in the So as long as you keep those temporary files around, the coffee-script files will load correctly in the dev tools. If you remove the temporary files, or run another build (even without installing), the source maps will either stop working or will start pointing to the wrong versions of the files. We could add a build flag to opt into this behavior, but it's a pretty fragile and hard-to-explain behavior. And seeing as how it would mostly get used during development, it seems like the existing Dev mode is a better solution to this problem. Thoughts? |
Conveniently coffee-script 1.11.0 was released two days ago, which adds support for inline source maps, which should solve problems with referencing source files from asar. |
I tried coffee-script 1.11.0 and inline sourcemaps do seem to work as intended. Made appropriate changes to this PR, requires #12780 to be merged first. |
@Ingramz This looks great. It looks like one
Could you take a look at that? |
@maxbrunsfeld should be good now. |
Thanks so much for investigating this @Ingramz! |
@maxbrunsfeld for this to kick in, I suspect that compile-cache needs to be cleared on CI. |
Great point @Ingramz. Done. |
Since sourcemaps still refer to
.coffee
files after build, we shouldn't be deleting those from final output directory. Debugging from devtools with latest builds is practically impossible without them.cc @as-cii
EDIT -
The approach has changed a bit since the PR was opened. See the comment below.