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

ignore invalid variable names during injection of global params #36

Merged
merged 1 commit into from
Jan 17, 2015
Merged

ignore invalid variable names during injection of global params #36

merged 1 commit into from
Jan 17, 2015

Conversation

Zolmeister
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 15dc51b on claydotio:you-dont-even-want-to-know-how-long-this-took-to-find into 79aac24 on jhnns:master.

@jhnns
Copy link
Owner

jhnns commented Jan 16, 2015

Thx for reporting this. That's a hard bug indeed. However, I think it's not valid to just ignore it. It would be better to use the bracket notation:

src += "var " + key + " = global[" + JSON.stringify(key) + "]; ";

@Zolmeister
Copy link
Contributor Author

I tried that first. Problem is that that's invalid too.

// Syntax Error
var a-b = ...

@Zolmeister
Copy link
Contributor Author

bump

@jhnns
Copy link
Owner

jhnns commented Jan 17, 2015

I see, you're right. I'll leave a note in the README about that.

jhnns added a commit that referenced this pull request Jan 17, 2015
…long-this-took-to-find

Ignore invalid variable names during injection of global params
@jhnns jhnns merged commit a23473b into jhnns:master Jan 17, 2015
@phishy
Copy link

phishy commented Feb 16, 2015

This is directly related to: #35

Here is a test case: https://gist.github.com/phishy/cb06c276a332b01ec149

It seems @Zolmeister wishes to test Sails.js also ;-) A fix for this is crucial to doing so when when using globally injected variables in Sails.

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.

None yet

4 participants