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

[[FIX]] Consistently lint evaluated code #3047

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jugglinmike
Copy link
Member

Previously, upon encountering certain forms of dynamic code evaluation
(namely the browser's setTimeout and setInterval functions), JSHint
would attempt to lint the code being evaluated. Due to what appears to
to be an unintentional typo on the JSHint source code, this behavior did
not extend to code evaluated via the Function and eval functions.

Extend this behavior to all forms of evaluated code.


@rwaldron: This is about 3 bugs separated from a more important issue, but I
want to tackle each part on its own. What do you think about this? It's
backwards-breaking, but the current behavior is clearly inconsistent and clearly
the result of a typo.

Previously, upon encountering certain forms of dynamic code evaluation
(namely the browser's `setTimeout` and `setInterval` functions), JSHint
would attempt to lint the code being evaluated. Due to what appears to
to be an unintentional typo on the JSHint source code, this behavior did
not extend to code evaluated via the `Function` and `eval` functions.

Extend this behavior to all forms of evaluated code.
@@ -2454,7 +2454,7 @@ var JSHINT = (function() {
left.value === "execScript") {
warning("W061", left);

if (p[0] && [0].id === "(string)") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's AMAZING.

@coveralls
Copy link

coveralls commented Oct 17, 2016

Coverage Status

Coverage increased (+0.03%) to 97.751% when pulling 35671ad on jugglinmike:lint-eval-code into bec152c on jshint:master.

@jugglinmike
Copy link
Member Author

Rick and I have discussed this offline, and we've decided that unfortunately,
we cannot include this fix outside of a major release of JSHint.

While it is a bug fix, and Semantic Versioning supports fixing bugs (even those
that are backwards-incompatible) with patch release, the important detail here
is that the underlying behavior is not documented. As far as I can see, there
is no wording in JSHint's documentation which explains how JSHint is expected
to lint any dynamically-modified code it encounters. Because of that, enabling
it in any new context (even though doing so would be more logical/consistent
overall) is effectively introducing a new feature.

In light of that, I'm going to label this fix for inclusion in the next major
release.

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

Successfully merging this pull request may close these issues.

None yet

3 participants