-
Notifications
You must be signed in to change notification settings - Fork 225
Passing tests again #61
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
Conversation
This fixes the tests, specifically for node v0.12. This notably reverts the fix for the uglify code. I will follow up with a pull request for that soon.
Node adds a header of 62 characters when code is required through a native module. If we are in node, the line number is 1 and the next file is the native 'module.js', we subtract that header. I verified the solution for evanw#36 by reproducing the problem. The stack trace is now: ``` user:node-source-map-support user$ node error.js 1 /Users/user/Documents/node-source-map-support/uglifysanity.js:2 goober(); ^ ReferenceError: goober is not defined at Object.<anonymous> (/Users/user/Documents/node-source-map-support/uglifysanity.js:2:1) ```
I don't understand why it is not always wanted to remove the headers (as you mentioned here), could you explain it in a few words? Also I think the name of |
I agree about |
This also makes the code more extensible for different offsets that may be required in the future.
I've updated the code to reflect your changes and hopefully be more extensible. Please merge and publish when convenient |
@@ -218,7 +218,7 @@ it('function constructor', function() { | |||
'throw new Function(")");' | |||
], [ | |||
'SyntaxError: Unexpected token )', | |||
/^ at Object\.Function \((?:unknown source|<anonymous>)\)$/, | |||
/^ at Function \((?:unknown source|<anonymous>|native)\)$/, |
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.
I don't understand this change, how is it related to #47?
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.
I think this change may be necessary due to some change in Node 0.12 and/or io.js, am I right?
If so, it should be move in its own PR.
Also, simply removing the Object\.
prefix may be breaking the tests on some platforms.
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.
This is relevant for Node 0.12. I'll move it into its own PR
Fixed in master, thanks. |
This fixes the tests, specifically for node v0.12. This notably reverts
the fix for the uglify code. I will follow up with a pull request for
that soon. This does mean we can push for #60