-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix tabbed Literate CoffeeScript #4345
Conversation
this should help to handle ligitimate markdown with indentation correctly
… for uniqueness, rather than a magic number that we hope might not occur in the code
I think it's fine to merge this into 2, but I'd leave master the way it is. (Kinda off-topic remark: In my opinion, there should be a 'literate-anything' package on npm that takes markdown as input and returns only the code of that markdown with a sourcemap. This literate thing hasn't got much to do with CoffeeScript, has it?) |
Yes, good point. I think the pieces of such a package are already here in this PR: just use |
It may fix the test, but including our first-ever dependency just for this is not so great. @lydell's approach would be nicer ;) |
The dependency doesn't just fix the test, it also fixes bugs related to false positives in recognizing code versus comments in .litcoffee files. So it has some merit on its own. But yes, maybe a better-still solution is to abstract out “literate anything” into a separate tool, and drop support for .litcoffee in the CoffeeScript compiler. Or the other way around, drop .litcoffee support and wait for someone else to build that tool or tools, if such a utility takes the form of a Gulp plugin, Webpack plugin etc. |
I'd vote for simply allowing dependencies in CoffeeScript 2.x. |
@lydell I kinda came to the same conclusion about literate being non-coffee specific, and created https://github.com/billymoon/illiterate to test the idea out, take a look :) |
That's fine too. |
@jashkenas I agree there is cost to introducing dependency, but in order to correctly implement literate feature, some kind of markdown parser is required, so either requiring dependency or including the code in repo. I expect even simple parser required to robustly parse markdown is sufficiently complex to be better off using externally maintained project. In the end, I think the optimal approach would be to remove the literate feature from coffeescript altogether, and to create a general literate anything tool. Perhaps this tool could be supported in coffeescript more generally via a plugin system, or pre/post-processor system of some kind!? Shell pipeline works fine too of course. |
That would indeed be ideal. It could be a soft dependency, and CoffeeScript could try to use it when it encounters a |
What is a “soft dependency,” and how could CoffeeScript use it conditionally? Isn’t a dependency either in And what’s the harm in having dependencies, in 1.x or 2? Aside from growing the browser version of the compiler, which no one should be using in production anyway. (Though one could argue that there’s no need for the browser version of CoffeeScript to know how to parse Literate CoffeeScript.) |
I merged the #3924 pull request onto the
2
branch and updated it slightly, to fix #3924’s inability to work in browsers and to replace a magic string with a generated one that we check for uniqueness. This fixes #4336 (the bug introduced by #4313).Per #3924, this uses the
marked
library to detect Markdown within.litcoffee
files, to avoid false-positive markdown matches by the CoffeeScript compiler. The reason #3924 was never merged was because it introduced a non-development dependency,marked
, which meant that the browser version of the CoffeeScript compiler was broken. I updatedCakefile
to includenode_modules/marked/lib/marked.js
as part of the browser build, so now the browser compiler works with #3924. This immediately makes the browser version of CoffeeScript 20K larger, but I don’t think anyone’s using the browser version of CoffeeScript outside of testing environments like http://coffeescript.org/#try or http://js2.coffee/ so it probably doesn’t matter.What do you all think? This at least fixes the broken test that plagues the
2
branch, is it worth merging intomaster
as well?