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

[CS2] Fix #4209: --require for filenames that are invalid identifiers #4658

Merged
merged 2 commits into from
Aug 23, 2017

Conversation

GeoffreyBooth
Copy link
Collaborator

Fixes #4209. I hate that this adds 15K to the size of the compiler, but fortunately the increase is only in the Node version, not the browser version (which doesn’t include command.coffee).

Previously --require hello-world.coffee would fail because the compiler was prepending hello-world = require('hello-world.coffee');. Now the compiler checks that hello-world is a valid identifier before outputting it; if it’s invalid, the compiler just outputs require('hello-world.coffee') without the assignment.

… required file/module is a valid identifier name before assigning to it; fixes jashkenas#4209
@GeoffreyBooth GeoffreyBooth requested a review from lydell August 23, 2017 06:15
@lydell
Copy link
Collaborator

lydell commented Aug 23, 2017

There are other places where we (in theory) need to check for valid identifiers as well, but, after much discussion, chose not to include that crazy regex.

Can't we output something like global['hello-world'] = require('hello-world.coffee'); instead?

@GeoffreyBooth
Copy link
Collaborator Author

D’uh. That’s a much better solution.

Copy link
Collaborator

@lydell lydell left a comment

Choose a reason for hiding this comment

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

Well, I can't think of anything breaking because of this.

@GeoffreyBooth GeoffreyBooth merged commit a3b08e1 into jashkenas:2 Aug 23, 2017
@jashkenas
Copy link
Owner

Whew!

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.

3 participants