CoffeeScript now supports ES2015 modules #7818
Conversation
…or whether to pass CoffeeScript’s output to Babel to look not just for backticks, but also for `import`, `export` and `function*`, the current ESNext features output by CoffeeScript
…mething exported by such a statement
| @@ -83,8 +83,9 @@ export class CoffeeCompiler extends CachingCompiler { | |||
|
|
|||
| let sourceMap = JSON.parse(output.v3SourceMap); | |||
|
|
|||
| if (source.indexOf('`') !== -1) { | |||
| // If source contains backticks, pass the coffee output through babel-compiler | |||
| if (source.match(/(`|import|export|function\*)/)) { | |||
There was a problem hiding this comment.
If I'm not mistaken, source is the contents of a CoffeeScript file (not JavaScript) and CoffeeScript doesn't have the function* generator syntax. Per CoffeeScript docs, they refer to it as "nonsense":
CoffeeScript functions also support ES6 generator functions through the
yieldkeyword. There's nofunction*(){}nonsense — a generator in CoffeeScript is simply a function that yields.
Therefore, this should be looking for yield instead of function*, no?
There was a problem hiding this comment.
Also, this regex could be improved (for example, with word boundaries so it doesn't match, say, a comment about reexportation) but it might be futile trying to match the CoffeeScript syntax accurately and efficiently.
Perhaps it would be better to just always pass it to babel-compiler? I'm not sure what the original thinking behind the conditional babel compilation was.
At the very least, if this stays there's no need for the outer parenthesis (as there is no capture) which just add extra, potentially-expensive steps for the regex engine.
There was a problem hiding this comment.
Good catch re function*. You are correct, it should be yield.
As for whether to use the regex at all . . . originally this was only checking for backticks, on the assumption that few if any CoffeeScript files had backticks. My Todos CoffeeScript example, for instance, has none. So checking for at least one backtick before recompiling the file through Babel was a performance improvement: only the very few files that needed it would get the second pass.
Now that the check is looking for import and export, many more files will get passed through Babel. Presumably the regex will short-circuit the moment it finds the first hit, and import statements are generally at the top of a file, so this regex should be quite inexpensive in most cases. But if most CoffeeScript files pass the test and get sent through Babel, then the regex might be hurting performance: basically, is the cumulative time spent running this regex for every file greater than the time saved by sparing a few files from double compilation?
My instinct is that the regex is still probably a net time saver, at least for now. There are probably still a lot of projects out there that don’t use modules, especially CoffeeScript projects. But I don’t feel strongly about it, if you think otherwise. I definitely think the check should be removed in a future release, probably at the next release of CoffeeScript.
There was a problem hiding this comment.
I don't have a good reproduction where I could reliably benchmark the two scenarios right now (perhaps that's something you might be in a better position to test?), but right now this would likely be adding a small amount of time (dependent on file-length) to most CoffeeScript projects because (as you said) most CoffeeScript projects will not be using modules right now and because of that, there will not be the ability to short-circuit early as each file will have to be read to completion. Unfortunately, that's just a trade off that'll be necessary to enable this module support.
| @@ -83,8 +83,8 @@ export class CoffeeCompiler extends CachingCompiler { | |||
|
|
|||
| let sourceMap = JSON.parse(output.v3SourceMap); | |||
|
|
|||
| if (source.match(/(`|import|export|function\*)/)) { | |||
| // If source contains backticks or ES2015+ features, | |||
| if (source.match(/`|\bimport\b|\bexport\b|\byield\b/)) { | |||
There was a problem hiding this comment.
The following should have a noticeable performance increase over this, especially on larger files:
source.match(/`|\b(?:import|export|yield)\b/)And if attempting to eliminate some false-positives, perhaps:
source.match(/`|(?:;|^)\h*\b(?:import|export|yield)\b/)Though I'd venture to say that it might be possible to break this logic – haven't given it a lot of thought. My assumption here is that import/export/yield are either at the beginning of a line or after a semi-colon, proceeded by optional whitespace.
Again, any benchmarks you could run here might be nice to see. Setting the env. variable METEOR_PROFILE=50 might be your friend in seeing the differences in compilation time.
There was a problem hiding this comment.
import or export by definition need to be at the beginning of a line, which in CoffeeScript means they follow a newline (no semicolons). yield would usually begin a line after some whitespace; I’m not sure if it could ever not start a line, but it might.
I can’t imagine it that takes more time to run this match than it does to recompile with Babel. Assuming most projects don’t use features that generate ES2015 code, which I think is a pretty safe assumption, this regex should be saving us time. And most projects that do generate ES2015 probably are doing so because of modules, which means this regex finds a match almost immediately, from the top of the file, so the match should be a very minor performance hit.
There was a problem hiding this comment.
import or export by definition need to be at the beginning of a line, which in CoffeeScript means they follow a newline (no semicolons)
Not common, but CoffeeScript actually can have semi-colons. But based on your description, my second regex above should work but would need the /m modifier. Again though, this only eliminates some potential false positives.
I can’t imagine it that takes more time to run this match than it does to recompile with Babel. Assuming most projects don’t use features that generate ES2015 code, which I think is a pretty safe assumption, this regex should be saving us time. And most projects that do generate ES2015 probably are doing so because of modules, which means this regex finds a match almost immediately, from the top of the file, so the match should be a very minor performance hit.
Agree, with the same assumptions. Though I'd still recommend the first regex change regardless.
There was a problem hiding this comment.
So which regex would you recommend? /|\b(?:import|export|yield)\b/`?
FWIW I think import is the most likely substring that a file would contain, and most likely to be first. Backticks are probably last.
There was a problem hiding this comment.
My vote would still be that, just for readability. The back-tick at the end is harder to see and the difference is extremely negligible that that point. 😄
|
@abernix I think we should be good. I don’t know why the CI tests sometimes pass and sometimes fail. |
|
Not sure about the failing tests anymore. |
|
The tests passed, then failed, and all I did was edit the regex slightly. I can’t imagine that my regex made any difference. It’s not failing on anything related to this package. |
|
Thanks for working on this, @GeoffreyBooth (and @abernix)! |
|
@GeoffreyBooth, so cool for this pull request. Could you also look into #7558 please? |
CoffeeScript 1.11.0 is out, and this release adds native support for
importandexportstatements. There’s no need to wrap such statements in backticks anymore.This pull request updates Meteor’s
coffeescriptpackage as follows:ecmascriptpackage for further compilation by Babel. Now CoffeeScript files that contain backticks or the substringsimport,exportorfunction*are passed toecmascript.importandexportstatements work.Regarding when to pass to
ecmascript, technically CoffeeScript currently has two features that are output as ES2015 rather than ES5: modules, and generator functions. It was a bug in the previous version of the package that generators were passed through to the runtime, rather than getting further processing byecmascript/Babel. We might want to consider eliminating this check and simply pass everything output by CoffeeScript toecmascript, rather than continuing to expand this list as CoffeeScript adds more features that it outputs in ESNext syntax. For now I’ve left it, since it should improve performance and the list of ESNext syntax features is still fairly short.