-
Notifications
You must be signed in to change notification settings - Fork 5.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
CoffeeScript now supports ES2015 modules #7818
Merged
Merged
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
79ad443
Update CoffeeScript to 1.11.0, which supports modules; update check f…
GeoffreyBooth 4c046fc
Add tests for imported modules using CoffeeScript’s new native import…
GeoffreyBooth e88abe6
Test that CoffeeScript native export statements work, by importing so…
GeoffreyBooth e1ac074
Improve regex
GeoffreyBooth 3dbc956
Optimize regex
GeoffreyBooth File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The following should have a noticeable performance increase over this, especially on larger files:
And if attempting to eliminate some false-positives, perhaps:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import
orexport
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 thematch
should be a very minor performance hit.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.
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.Agree, with the same assumptions. Though I'd still recommend the first regex change regardless.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😄