-
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
ES module version of compiler for use in browsers; dynamic import() docs; revised Stage 3 policy #5177
ES module version of compiler for use in browsers; dynamic import() docs; revised Stage 3 policy #5177
Conversation
eb63da1
to
3c6d609
Compare
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.
@GeoffreyBooth I'm not really familiar with the docs setup or the browser compiler but this lgtm
@@ -58,7 +58,7 @@ CoffeeScript.load = (url, callback, options = {}, hold = false) -> | |||
# Activate CoffeeScript in the browser by having it compile and evaluate | |||
# all script tags with a content-type of `text/coffeescript`. | |||
# This happens on page load. | |||
runScripts = -> | |||
CoffeeScript.runScripts = -> |
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.
What prompted exposing runScripts()
?
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 was thinking that the browser compiler (or the ES module version, at least) shouldn’t have side effects merely by being imported into the page. Like if the user wants to register an event handler for CoffeeScript to transpile and evaluate all <script type="text/coffeescript">
blocks, the user should have to do so explicitly; it shouldn’t happen automatically. Especially now when those blocks could be either Script or Module code (i.e. they could use import
statements) and the compiler doesn’t know that from just the <script type="text/coffeescript">
tag.
I just checked and apparently I’m still having runScripts
run on load rather than requiring registering, so I need to fix this. It’s attached to CoffeeScript
so that it can be exported (see the const { VERSION, compile, eval: evaluate, load, run, runScripts } = CoffeeScript;
line).
@@ -1113,11 +1113,7 @@ exports.isUnassignable = isUnassignable | |||
# “sometimes” keyword. | |||
isForFrom = (prev) -> | |||
if prev[0] is 'IDENTIFIER' | |||
# `for i from from`, `for from from iterable` | |||
if prev[1] is 'from' | |||
prev[1][0] = 'IDENTIFIER' |
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.
@helixbass This was throwing in Module mode only, because it tried to assign 'IDENTIFIER'
to a character in a string: prev[1]
is the string 'from'
, so prev[1][0]
is the character f
in that string. In Script mode this just silently fails, whereas in Module it throws.
It appears this isn’t needed at all, as removing it doesn’t break any tests. I don’t remember what I was thinking. Does this all seem correct to you?
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.
@GeoffreyBooth it does make sense that assigning to prev[1][0]
wouldn't have been correct
Tracing the possibilities from the comment, it also looks correct that this logic wasn't needed:
for from from iterable
is effectively already covered by the else if prev[0] is 'FOR'
clause below (as implied by its comment # `for from…`
for i from from
won't actually make it this far when checking its second from
(isForFrom()
won't get called) since @seenFor
will have been set to no
when the first from
is checked
So I'd recommend just updating and moving the comment:
# `for i from iterable`
if prev[0] is 'IDENTIFIER'
yes
# `for from…`
...
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.
Aside from the placement of the comment, I think what I have now matches what you’re suggesting?
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 was also suggesting removing `for i from from`
and `for from from iterable`
from the comment since they're not actually covered by this case
…* throws in Module mode (and silently does nothing in Script mode) so remove this unneeded code that throws when compiling `fn for i from from iterable` in Module mode
…y attach the runScripts event handler
ad7505f
to
c42d3ed
Compare
I started writing the docs for dynamic import (#5169) and I wanted to include a runnable example. There aren’t too many ES module scripts available for use out on the Web, and I wanted one where there wasn’t magic or rewriting going on like sometimes happens at Pika or unpkg, so I decided to create an alternate build of the CoffeeScript browser-based compiler that works as an ES module.
So now in the
package.json
there’s a"module"
field that points to a version of the CoffeeScript compiler that uses anexport
statement to provide the browser compiler’s public API to anyone who wants to consume it in a browser<script type="module">
environment. When support for ES modules comes to Node, I’ll create an ES module build for use in Node as well.See the new ES module CoffeeScript compiler in action:
Besides that, this PR includes docs for
import()
and an update to the Contributing section amending our policy on new features to allow Stage 3 features that have been released in major browsers and/or Node (with the caveat that they should be considered experimental until the features reach Stage 4).cc @helixbass @voxsoftware @jashkenas