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

Add support for text/literate-coffeescript in the browser #2718

Merged
merged 1 commit into from Feb 28, 2013

Conversation

sbp
Copy link

@sbp sbp commented Feb 25, 2013

This branch allows users of the browser-ready extras/coffee-script.js file to embed Literate CoffeeScript in web pages. For example:

<script src="extras/coffee-script.js"></script>
<script type="text/literate-coffeescript">
This is a Literate CoffeeScript example.

    console.log "Success!"
</script>

This results in "Success!" being displayed in the JavaScript console.

Contributors are urged by CONTRIBUTING.md to add their own tests. There are, however, no existing tests for the browser scripts functionality except for the test wrapper itself in tests/test.html For that reason, I have not added any new tests. I have of course tested that the code works on the web.

EDIT: The original submission used text/coffeescript+literate. This was changed to text/literate-coffeescript in a later squashed commit due to a suggestion from @jashkenas.

@michaelficarra
Copy link
Collaborator

Eh, +1. We might as well support it if we have it. Please commit the built /lib/coffee-script/browser.js and squash.

@sbp
Copy link
Author

sbp commented Feb 27, 2013

Thanks, done as instructed. Force-pushed the squashed commit.

@@ -38,16 +38,19 @@ CoffeeScript.load = (url, callback) ->
# This happens on page load.
runScripts = ->
scripts = document.getElementsByTagName 'script'
coffees = (s for s in scripts when s.type is 'text/coffeescript')
coffeetypes = {'text/coffeescript': true, 'text/coffeescript+literate': true}
coffees = (s for s in scripts when s.type of coffeetypes)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something wrong with

coffeetypes = ['text/coffeescript', 'text/coffeescript+literate']
coffees = (s for s in scripts when s.type in coffeetypes)

?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, either way would be fine by me.

I avoid array indexing for lookups out of habit, thinking of hash lookups as more efficient.

: 1 instead of : true would be fine too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler will optimize this for you:

$ coffee -bep 's.type in ["text/coffeescript", "text/coffeescript+literate"]'
var _ref;

(_ref = s.type) === "text/coffeescript" || _ref === "text/coffeescript+literate";

@jashkenas
Copy link
Owner

How about text/literate-coffeescript instead?

@sbp
Copy link
Author

sbp commented Feb 28, 2013

@jashkenas: text/literate-coffeescript is okay with me.

The +suffix convention started recently. RFC 4288 does caution to use it sparingly, and there could have been a case to use +markdown instead of +literate anyway.

I'll submit a new squashed commit using text/literate-coffeescript, and incorporating the hash to array suggestion, if nobody objects.

@jashkenas
Copy link
Owner

Please do.

@sbp
Copy link
Author

sbp commented Feb 28, 2013

Done. Tested the cake build:browser output too.

jashkenas added a commit that referenced this pull request Feb 28, 2013
Add support for text/literate-coffeescript in the browser
@jashkenas jashkenas merged commit 3f23be2 into jashkenas:master Feb 28, 2013
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.

None yet

5 participants