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

create-test / default-params issue #55

Closed
xjamundx opened this issue Feb 9, 2015 · 4 comments
Closed

create-test / default-params issue #55

xjamundx opened this issue Feb 9, 2015 · 4 comments
Labels

Comments

@xjamundx
Copy link
Contributor

xjamundx commented Feb 9, 2015

  1. the placeholder for empty defaults array members should possibly be undefined instead of null
  2. create-test-.js improperly changes undefined array members to null.

Recently when working on defaultParams and destructuring I noticed that I was having some mismatches with Esprima on the expected output, especially involving the placeholder when there are not defaults specified for each parameter.

When I use create-test to generate the expected output it seems to show [null, 1] for function(a, b=1){}, but when i actually console.log the output from Esprima, it looks like [undefined, 1].

Here is I think the source of the issue:
screen shot 2015-02-09 at 7 00 23 am

The only thing that makes me unsure is that the single test in esprima also shows null as the placeholder, but I can only assume they're using JSON.stringify in their tests, because Esprima will not generate that output:
https://github.com/jquery/esprima/blob/harmony/test/harmonytest.js#L2612

Let me know if you have any ideas otherwise I'll like manually change the tests to use undefined as part of #51, which changed some of the code around this area and caused me to bump into this issue.

@nzakas
Copy link
Member

nzakas commented Feb 9, 2015

Ah, interesting. Was this causing an error during testing? Since null == undefined, it would seem like a semantic difference but not a functional one.

@nzakas nzakas added the question label Feb 9, 2015
@xjamundx
Copy link
Contributor Author

xjamundx commented Feb 9, 2015

When I updated to the latest eprima version of parseParam method for the destructuring feature, the old defaultParams tests started failing. The harmony version looks like this:
https://github.com/ariya/esprima/blob/harmony/esprima.js#L4586

options.defaults.push(def);

In order to not break tests I had to:
https://github.com/eslint/espree/pull/51/files#diff-7a476a3203e745831344f37dd24a8a97R4293

options.defaults.push(def ? def : null);

And yes I checked everywhere upstream and made sure there was no setting of null anywhere else. I actually have a hunch that there used to be some sort of explicit nulling happening, but that it was removed and for whatever reason the esprima tests still passed, whereas ours are stricter or something.

Kind of weird thing, I'll go with whatever is the path of least resistance for now, which will be to leave the def ? def : null fix in there and then maybe I can check with esprima and figure out what is the official approach.

@xjamundx
Copy link
Contributor Author

Probably it doesn't really matter, because it's such an edge case, but if we want to fix the create-test tool we may be able to use util.inspect instead of JSON.stringify. Here is what the difference is like:

var x = {
    a: [undefined, undefined],
    b: {x: undefined }
};

util.inspect(x, {depth: null}) // { a: [ undefined, undefined ], b: { x: undefined } }

JSON.stringify(x); // {"a":[null,null],"b":{}}

The only issue I'm seeing is that I'm not sure how to format util.inspect for multiple lines.

http://nodejs.org/api/util.html#util_util_inspect_object_options

@xjamundx
Copy link
Contributor Author

Realizing this is a super small edge case and not likely worth fixing, especially as the way default params are being handled is changing anyway jquery/esprima#1081

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants