src: fix builtin modules failing with --use-strict #9237
Conversation
|
As @cjihrig pointed out, it's been implemented in io.js with nodejs/node@21130c7. I think there is still some value in keeping the test added by this PR, as it makes sure that any new built-in module that could be added in the future works in strict mode. Also, nodejs/node@21130c7 fixes the What do you think? |
|
I'm all for adding |
misterdjules
force-pushed
the
fix-issue-9187
branch
2 times, most recently
from
4cbe91a
to
c0bb576
Compare
|
@cjihrig Added misterdjules@c0bb576 to this PR according to your comments. |
|
@tjfontaine @trevnorris @cjihrig @orangemocha @srl295 @jasnell @mdawsonibm I added this to the0.12.1 milestone, but I would like to get your thoughts on doing so. |
| * Wait for both `finish` and `end` events to ensure that all data that | ||
| * was written on this side was read from the other side. | ||
| */ | ||
| CryptoStream.prototype._destroyWhenReadAndWriteEndsDone = function() { |
There was a problem hiding this comment.
I don't think we really need to expose another method on the prototype just to deal with the finish() function.
|
I think it should be fine to go into 0.12.1. |
misterdjules
force-pushed
the
fix-issue-9187
branch
from
c0bb576
to
4fc03c6
Compare
|
@cjihrig I realized that enabling Failure to be strict-mode compliant is reported at runtime only when non-compliant code is executed, not when it's loaded. Thus, in order to be confident that enabling strict mode won't break a lot of existing applications, we'll need to make such a change sit in the code base for a while, let users report issues and fix them. The current tests suite passes with all built-ins modules in strict mode, but it's not sufficient since it doesn't have 100% code coverage. Even if we reached such a number, there could still be a common code path/input/state combination that would break strict-mode and that is not tested by the tests suite. For this reason I would suggest enabling strict-mode in all built-in modules in master and not in v0.12. What do you think? |
|
It's your call. My personal preference would be to put it in 0.12.1, but I understand your concern. |
|
@cjihrig Thank you for your feedback! To illustrate my point and to provide more context, I broke node-inspector (and probably other code) by enabling strict mode in That change broke node-inspector even though the file was loading fine and the built-in debugger was working without any issue. The reason I hadn't seen the problem is that node-inspector simply has a different coverage than the built-in debugger or any of the tests in node's tests suites. I will keep the PR as it is for the v0.12 branch, and submit another PR targeted to master that enables strict mode in all built-in modules. |
|
@cjihrig Could you please rubber-stamp it if you're ok with merging the current PR in v0.12? Thank you! |
|
LGTM |
1 similar comment
|
LGTM |
Currently, lib/_tls_legacy.js and lib/crypto.js cannot be loaded when --use-strict is passed to node. In addition to that, console.trace throws because it uses arguments.callee. This change fixes these issues and adds a test that makes sure every external built-in module can be loaded with require when --use-strict is enabled. Please note that this change does not fix all issues with built-in modules' code running with --use-strict. It is very likely that some code in the built-in modules still fails when passing this flag. However, fixing all code would require us to enable strict mode by default in all builtins modules, which would certainly break existing applications. Fixes nodejs#9187.
misterdjules
force-pushed
the
fix-issue-9187
branch
from
5872199
to
7eaeddb
Compare
|
@cjihrig @trevnorris Thanks for the review! The tests suite failed for |
misterdjules
force-pushed
the
fix-issue-9187
branch
from
7eaeddb
to
d51f79e
Compare
|
All tests passing and removed the additional commit, will land soon. |
Currently, lib/_tls_legacy.js and lib/crypto.js cannot be loaded when --use-strict is passed to node. In addition to that, console.trace throws because it uses arguments.callee. This change fixes these issues and adds a test that makes sure every external built-in module can be loaded with require when --use-strict is enabled. Please note that this change does not fix all issues with built-in modules' code running with --use-strict. It is very likely that some code in the built-in modules still fails when passing this flag. However, fixing all code would require us to enable strict mode by default in all builtins modules, which would certainly break existing applications. Fixes nodejs#9187. PR: nodejs#9237 PR-URL: nodejs#9237 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Currently, lib/_tls_legacy.js and lib/crypto.js cannot be loaded when --use-strict is passed to node. In addition to that, console.trace throws because it uses arguments.callee. This change fixes these issues and adds a test that makes sure every external built-in module can be loaded with require when --use-strict is enabled. Please note that this change does not fix all issues with built-in modules' code running with --use-strict. It is very likely that some code in the built-in modules still fails when passing this flag. However, fixing all code would require us to enable strict mode by default in all builtins modules, which would certainly break existing applications. Fixes #9187. PR: #9237 PR-URL: #9237 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>
|
Landed in b233131, thank you! |
|
Can this be closed? |
|
Yes, closing now. |
Currently, lib/_tls_legacy.js and lib/crypto.js cannot be loaded when
--use-strict is passed to node. This change adds a test that makes sure
every external built-in module can be loaded with require when
--use-strict is enabled, and fixes the built-in modules that are
currently broken.
Fixes #9187.