Error on strict mode

[lewispham]

I got this error with the code expect(value).to.be.null that run on strict mode.

P/s. I'm using chai@3.4.1 installed via npm install chai.

[keithamus]

Thanks for the issue @Tresdin.

How are you running these tests? Through Mocha? Through Karma? Are you using CommonJS style require? Or are you dropping in <script> tags onto a page? It looks as though you're probably concatenating Chai with something else that declares "use strict".

[lewispham]

@keithamus

How are you running these tests? Through Mocha? Through Karma?

I'm using Mocha.

Are you using CommonJS style require?

Yes. I am.

Or are you dropping in <script> tags onto a page

No. This code is run on nodejs. This error doesn't occur when I run chai on browsers

It looks as though you're probably concatenating Chai with something else that declares "use strict".

I run node globally on strict mode (via --use_strict flag)

[keithamus]

I run node globally on strict mode (via --use_strict flag)

There's your problem! Remove that flag and things will work fine! Not every module you use will implement strict code (for example, Chai does not!). You'd be better off declaring "use strict"; at the top of every file that you want to use strict parsing.

I'm going to close this because we rely on non-strict features (which are still compliant to the JS spec, just not the "strict" part of the spec). It's unlikely we'll change this any time soon, and only errors through edge cases (such as enforcing strict mode on all of your modules, or using babel, etc).

[lewispham]

@keithamus Bad news, indeed. I think you should seriously consider this issue as a critical bug. chai should not fail on strict mode.

[keithamus]

@Tresdin I'm curious; why should chai work in strict mode? "Strict Mode" is an opt in part of JavaScript and as such is not a requirement for engines (or libraries) to implement. Some engines can enforce strict mode (e.g. via --use_strict) which is arguably non-compliant as not all code has to be strict.

Just FYI, the only "non-strict mode" feature (which is a little bit of a misnomer, because it's really a "JavaScript feature which throws an Error in Strict Mode") we use is arguments.callee on L33 of assertion.js. This is a useful feature because it allows us to trim superfluous stack trace information, hence why Chai comes with such nice stack traces and you don't see a dozen lines of chai internals when an assertion fails. If we got rid of arguments.callee you'd lose this nice feature.

I suppose if you could find a way around using arguments.callee and still retain the stack trace functionality (without using hacks like popping the stack trace), then I'd happily merge the solution and we'd no longer have code which throws in strict mode. As usual, if you submit a PR you get a place on our wonderful hall of fame, and of course you'd get to use node --use_strict 😄.

[lewispham]

@keithamus

why should chai work in strict mode?

Same reasons why any other piece of Javascript code should work in strict mode.
According to John Resig:

• It catches some common coding bloopers, throwing exceptions.
• It prevents, or throws errors, when relatively “unsafe” actions are taken (such as gaining access to the global object).
• It disables features that are confusing or poorly thought out.

According to MDN:

Strict mode makes several changes to normal JavaScript semantics. First, strict mode eliminates some JavaScript silent errors by changing them to throw errors. Second, strict mode fixes mistakes that make it difficult for JavaScript engines to perform optimizations: strict mode code can sometimes be made to run faster than identical code that's not strict mode. Third, strict mode prohibits some syntax likely to be defined in future versions of ECMAScript.

So in brief, you will end up writting better, cleaner and even faster code in strict mode.

About the solution, I'm not sure what is the difference between

//fails in strict mode
  function Assertion (obj, msg, stack) {
    flag(this, 'ssfi', stack || arguments.callee);
    flag(this, 'object', obj);
    flag(this, 'message', msg);
  }

and

//works in strict mode
  function Assertion (obj, msg, stack) {
    flag(this, 'ssfi', stack || Assertion);
    flag(this, 'object', obj);
    flag(this, 'message', msg);
  }

since arguments.callee is just a reference to the currently executing function.

[btd]

Also es modules implicitly add 'use strict'.

[sholladay]

Strict mode is only opt-in because it would wreak havoc on the web to make it suddenly mandatory. Not because its behavior is unimportant.

[keithamus]

I'd like to quickly reiterate:

• We are aware of the benefits of strict mode.
• All but one line is strict "compatible" code. For all but assertion.js - the only thing that doesn't make them definitively strict mode is the missing stanza.
• You don't need chai to be strict code, you can import non-strict code into your strict-mode codebases without problem.
• If you would like to find a workaround for the one line we have, fantastic! Please send a PR and we'll gladly merge it 😄

[ghost]

@keithamus what about the solution @Tresdin mentioned?

flag(this, 'ssfi', stack || Assertion)

Is there something wrong with this? A quick test shows me that it removes the internal function calls from the shown stack trace.

I use --use_strict param for node because I'm lazy, and I always forget (and don't really like) the "use strict" at the top of my every file.

[keithamus]

@bezformy I don't know if that solution will be ideal or not - the ssfi part of the codebase is the one I (personally) have the least confidence with. It could indeed be sufficient, as most assertions go through the addMethod, overwriteMethod, addProperty and overwriteProperty methods, which set the SSFI flag.

If you'd like to make a PR for that implementation, please do so. But also if you could add some tests to show that it does work the same as the old behaviour so we can be sure. That'd be awesome 😄.

[lewispham]

@keithamus I think you're overthinking the problem. The only test that you need to write for the change is something similar to this code.

function Assertion(){
     assert.equals(Assertion,arguments.callee,'Mr. Internet Explorer, is it you?');
}
//case 1
Assertion();
//case 2
new Assertion();

Why should you bother any other code if Assertion and arguments.callee are identical?

[keithamus]

@Tresdin Assertion and arguments.callee are not identical. arguments.callee refers to the function that called Assertion. More to the point - if we're changing this code, we should have tests to back up the change, to prevent regressions of both existing behaviour, and any future changes to the code. Currently this area of code is not properly covered by tests, so I am asking that anyone who makes the PR add tests to ensure that they are. I would expect a PR to have tests which assert that the stack does not include internal function calls (which is the behaviour now).

[ghost]

You mean arguments.callee.caller (or Function.caller)?

The function.caller property returns the function that invoked the specified function.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/caller

There must be some kind of misunderstanding here, because arguments.callee is a bit different:

callee is a property of the arguments object. It can be used to
refer to the *currently* executing function inside the function
body of *that* function.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/arguments/callee

[keithamus]

Ahh wow, my bad. I suppose Assertion and arguments.callee are identical! Sorry for the misinformation and sorry to @Tresdin for disagreeing.

I suppose the ssfi || Assertion pattern should work well then. I'd still strongly reiterate that I'd like to see tests to back this up. Again though, if anyone wants to make a PR - please feel free.

[ghost]

@keithamus I checked the code to see what is tested and what isn't, and I found that this does what you are asking for:

chai/test/configuration.js

Line 55 in 5608a0b

assert.notInclude(err.stack, 'assertEqual', 'should not have internal stack trace in error message');

It basically checks if any internal function calls are included in the stack trace or not. Is there anything else that needs to be checked?

[keithamus]

@bezformy good find! That should actually be enough. In that case I welcome a change of just the one line, no extra tests will be needed.

Perhaps, though, as part of this change, each file should be given the 'use strict'; stanza to verify that they are all indeed, opting into strict mode?

[ghost]

I checked a few things and I think it might be best if the 'use strict'; were not included.
http://stackoverflow.com/questions/16871050/inconsistent-scope-of-use-strict-on-different-web-browsers-concerning-argumen
The linked jsfiddle fails in Firefox, so if somebody extends chai in some weird way it could cause problems for them.

[lucasfcosta]

Hey @bezformy, thanks for your contribution, the link you've just posted is extremely interesting 😄

Well, so let me summarize this and simplify the question we've came to here.

In a nutshell, the error described on the post you linked is due to getting a strict function using .caller, as you can read on this paragraph:

What's interesting about all of the above is that in addition to the clearly-specified behavior of throwing an error if you try to access caller on strict functions, Chrome, Firefox, and IE10 all (in various ways) prevent your using caller to get a reference to a strict function, even when accessing caller on a non-strict function.

The only way this could cause a bug would be if someone used arguments.caller to get a strict function from Chai. IMO this is an extremely rare case, but it could still happen.

EDIT: I clearly over-thought this so I edited this to include only the correct and useful part.
Please see the excelent explanation by @bezformy below. That's what I think may be the best solution for this.

[ghost]

Hey @lucasfcosta, I'm not sure I understand this. What is exactly the final decision then? I think the arguments.callee still should be replaced with:

flag(this, 'ssfi', stack || Assertion)

The only thing I wouldn't include is the 'use strict'; part on the top of every file. This way no bugs would be introduced, everything would still work as expected and lazy people like me could use --use_strict without any error message.

[lucasfcosta]

@bezformy Ah, I see, I totally agree with you and I think your solution to this is excellent, thanks for explaining.
That makes a lot of sense, replacing .callee with Assertion seems good to me and there would be no downsides to this change. I clearly over-thought this.

Anyway, there's no final decision here, I'd like to hear everyone's opinion on what they think is better 😄.

[sholladay]

I would add the strict pragma. Whereas the upsides are very practical, the downsides are only hypothetical. And if Chai's philosophy is we shouldn't activate strict mode because it might bite somebody, well, that means using ES modules is also out of the question. The only future-proof approach here is to add it.

[keithamus]

Long term we'll be splitting out most of the code here into separate modules, at which point they'll all be tidied up and very likely have the 'use strict' stanza. So it's essentially inevitable that all chai code will be strict mode code.

Having said that, it's not something we need to rush into and it seems to be holding up the actual fix to this. So I suggest this becomes the following:

• the one line test @bezformy mentions: flag(this, 'ssfi', stack || Assertion)
• no other changes
• no additional tests