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

CoffeeScript now supports ES2015 modules #7818

Merged
merged 5 commits into from Sep 28, 2016

Conversation

Projects
None yet
4 participants
@GeoffreyBooth
Contributor

GeoffreyBooth commented Sep 24, 2016

CoffeeScript 1.11.0 is out, and this release adds native support for import and export statements. There’s no need to wrap such statements in backticks anymore.

This pull request updates Meteor’s coffeescript package as follows:

  • The version of CoffeeScript is bumped to 1.11.0.
  • Previously, any CoffeeScript file that contained at least one backtick was passed to the ecmascript package for further compilation by Babel. Now CoffeeScript files that contain backticks or the substrings import, export or function* are passed to ecmascript.
  • New tests are added to check that the native import and export statements 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 by ecmascript/Babel. We might want to consider eliminating this check and simply pass everything output by CoffeeScript to ecmascript, 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.

GeoffreyBooth added some commits Sep 24, 2016

Update CoffeeScript to 1.11.0, which supports modules; update check f…
…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
@@ -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\*)/)) {

This comment has been minimized.

@abernix

abernix Sep 26, 2016

Member

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 yield keyword. There's no function*(){} nonsense — a generator in CoffeeScript is simply a function that yields.

Therefore, this should be looking for yield instead of function*, no?

This comment has been minimized.

@abernix

abernix Sep 26, 2016

Member

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.

This comment has been minimized.

@GeoffreyBooth

GeoffreyBooth Sep 26, 2016

Contributor

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.

This comment has been minimized.

@abernix

abernix Sep 26, 2016

Member

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/)) {

This comment has been minimized.

@abernix

abernix Sep 26, 2016

Member

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.

This comment has been minimized.

@GeoffreyBooth

GeoffreyBooth Sep 26, 2016

Contributor

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.

This comment has been minimized.

@abernix

abernix Sep 26, 2016

Member

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.

This comment has been minimized.

@GeoffreyBooth

GeoffreyBooth Sep 26, 2016

Contributor

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.

This comment has been minimized.

@abernix

abernix Sep 26, 2016

Member

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. 😄

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Sep 26, 2016

@abernix I think we should be good. I don’t know why the CI tests sometimes pass and sometimes fail.

@abernix

This comment has been minimized.

Member

abernix commented Sep 27, 2016

I think the tests on this are failing due to an unrelated errorseemingly because of npm/npm-registry-client#142 and npm/npm#13656.

I'll resubmit this to CircleCI.

Not sure about the failing tests anymore.

@GeoffreyBooth

This comment has been minimized.

Contributor

GeoffreyBooth commented Sep 27, 2016

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.

@benjamn benjamn merged commit 12472cf into meteor:devel Sep 28, 2016

3 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benjamn

This comment has been minimized.

Member

benjamn commented Sep 28, 2016

Thanks for working on this, @GeoffreyBooth (and @abernix)!

@mitar

This comment has been minimized.

Collaborator

mitar commented Sep 30, 2016

@GeoffreyBooth, so cool for this pull request. Could you also look into #7558 please?

@GeoffreyBooth GeoffreyBooth deleted the GeoffreyBooth:coffeescript-modules branch Oct 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment