Move Node and ES6 rules to relevant sections; other minor clarifications

[brettz9]

No description provided.

[mention-bot]

By analyzing the blame information on this pull request, we identified @BYK, @btmills and @gcochard to be potential reviewers

[eslintbot]

Thanks for the pull request, @brettz9! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.
• Pull requests with code require an issue to be mentioned at the end of the commit summary, such as (fixes #1234). Please update the commit summary with an issue (file a new issue if one doesn't already exist).
• Please sign our CLA. This is just a way for you to say that you give us permission to use your contribution.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[BYK]

Hi @brettz9, this looks pretty cool but I think it should be split up into multiple PRs:

1. Rearranging rules for better categories
2. no-unused-expressions clarification for strict mode
3. Minor fixes in no-new-func
4. Replace all node references to Node.js which is the official name

You also need to format your commit message according to @eslintbot's response.

[michaelficarra]

I don't agree with this addition. First, all directives are not considered problems. Second, the fact that directives are not reported should be obvious because directives are not expressions and this rule only ever reports expressions. Are we going to say that if statements are not problems? Function declarations? Import/export declarations? do-while statements? Lexical declarations? No, none of these need to be individually called out.

[platinumazure]

Does our parser actually call out directives vs ExpressionStatements? If not, I think your suggested language will be even more confusing.

[pedrottimark]

@platinumazure Here is the relevant code from this rule:

function looksLikeDirective(node) {
    return node.type === "ExpressionStatement" &&
        node.expression.type === "Literal" && typeof node.expression.value === "string";
}

It is consistent with the way that Douglas Crockford describes strict mode directive in a classic blog article
http://yuiblog.com/blog/2010/12/14/strict-mode-is-coming-to-town/

“It has the form of a useless string literal statement, so it will be ignored by ES3 systems.”

When I first read this rule page a few months ago, the question did come to my mind: what about a "use strict" string? Looking for an explicit answer distracted me for a moment. Sure, I soon decided that the rule would have to ignore them.

So what y’all think about a sentence at the end of Rule Details? See lines-around-comment and no-implicit-global.

Here is a first draft for comment:

This rule does not apply to directives (which are in the form of literal string expressions) such as "use strict".

[platinumazure]

I like that a lot better.

[brettz9]

As far as all directives being permitted, yes, @michaelficarra, I myself also would favor the more generic wording presented by @pedrottimark.

(Incidentally, however, I think a new issue should be created to flag directives except strict mode and those whitelisted, such as "use asm". Otherwise, it is possible someone may mistype and get unexpected code behaviors or fail extra validation. While the "strict" rule covers this for strict mode, maybe this rule should be merged into such a proposed general purpose one. I haven't looked through the code carefully, but it also looks like the code may fail to block usage of EscapeSequence and LineContinuation which are not permitted for a strict directives.)

As far as it being "obvious", @michaelficarra, that directives are not expressions, while this may be true to those who have read the spec, presumably this tool is intended to help everyone, including those who are not familiar with every bit of arcana in the spec (how often do "directives" as a category come up, anyways? Yes strict mode is common, but one might assume it (and directives) are just hacks used by ES engines rather than features explicitly anticipated in the language, and indeed they were before ES5). For those who haven't read the relevant parts of the modern specs, a string expression statement at the top of a module, script, or function, is indistinguishable from an unused expression (statement). There is no need to explicitly mention different kinds of statements and declarations as non-problematic because it is clear to those with some background in the language that they are very much central to the language and have a function.

Moreover, unlike directives, statements (except expression statements, of course, which are already addressed) are also not normally syntactically confusable with expressions by anyone who has some idea of the difference between the two, so these do not need to be called out as being permissible. Of the two possible exceptions:

1. A goofy block statement containing a label and statement (that without context is indistinguishable from a single-key object literal) perhaps also ought to be called out as not being a problem, but it is hardly likely that this will even occur as a possibility to most humans, and there is already an example (unfortunately without comment) indicating that an empty block statement, which can be confusable with an object literal, is not considered a problem.
2. An unused named function might be worth calling out as an example of something that will nevertheless be flagged as a problem when clearly used as an expression (since it can be confusable with a function declaration and people might wonder whether the linting code naively glosses over this distinction), but as its form as a declaration is well known as having utility in all environments, people will not need to see function declarations explicitly called out as not being problematic (though reference might still be made to the no-unused-vars rule).

Even for those who have read the spec, particularly due to the fact that "Hello world" is used as an example of an unused expression--without indicating that it is only considered as such if not in a prologue position (where it would then be a directive)--it may lead them to question whether we might be being loose in our definitions.

If an apartment contract states, "Allowable pets include birds", a conscientious tenant might still ask whether an ostrich or penguin is permissible, even if it is technically "obvious" that it is included since many landlords would not consider these as allowable, despite the "letter of the law". In many semantic concepts, there are often those consistent with typicality and those which are outliers (see https://en.wikipedia.org/wiki/Exemplar_theory ), and taking this into account helps us clarify things for users (and without inviting excessive questions). If using the spec terms alone were sufficient for the average person, we wouldn't need to have any examples at all.

[pedrottimark]

@brettz9 You mentioned a potential whitelist of directive strings. Could you take a look at the ES5, ES2015, and ES2016 specs to see what they say about the interpretation in earlier versions of directives added in later versions? Also whether Mozilla Developer Network has anything about what JavaScript engines might do with directives outside the ECMAScript spec?

What is in the back of my Friday evening mind is when people would (or should) need to change their ecmaVersion in ESLint for new directives? Also, is "use asm" defined in ECMAScript or is it a feature of JavaScript engines?

[brettz9]

As far as "interpretation in earlier versions of directives added in later versions", the specs don't seem to have varied much. As per ES2015, ES2016, and the latest draft and also back in ES5 with one word since removed ("during evaluation of the containing SourceElements production"), there is the following note:

The ExpressionStatement productions of a Directive Prologue are evaluated normally during evaluation of the containing production. Implementations may define implementation specific meanings for ExpressionStatement productions which are not a Use Strict Directive and which occur in a Directive Prologue. If an appropriate notification mechanism exists, an implementation should issue a warning if it encounters in a Directive Prologue an ExpressionStatement that is not a Use Strict Directive and which does not have a meaning defined by the implementation.

So it appears, at most, a warning will be issued.

Looking around at MDN and also on StackOverflow, etc., I only see "use asm" there. However, out in the wild, I do see a number of others mentioned...

For compiling from different ES versions:

'use babel';
'use 6to5';

For Google's SoundScript:

'use stricter';
'use stricter+types';

Only strict mode has been defined in ECMAScript up through the 2017 draft, so "use asm" is just a feature of compliant engines.

Checking ecmaVersion does sound useful in case new official directives are added, but as per the above examples, these may vary not correspond directly to new versions. I'd expect one will only need to be concerned with what has been whitelisted unless some new mode which changed behavior came along (and that might encounter some opposition akin to DOCTYPE sniffing).

Thankfully, with the exception of 'use strict', my quick impression is that the others do not seem to actually change behavior, but either flag the code for conversion or indicate an optimized subset of JavaScript.

In the case of ASM, this subset of JavaScript is unlikely to be added by hand or shown in a non-minified copy since it is typically compiled (e.g., from C).

As an FYI, "use " is not required for directives. Any string in a prologue context can serve as one.

[pedrottimark]

@brettz9 Thank you for the quick research results!

Hey, what do you think about Node.js suggested as item 4 in #5451 (comment) as the subject for a separate pull request? The occurrences I found in docs/rules are in a bullet list at the end of this comment.

1. If you have not yet signed the simple Contributor License Agreement (CLA) go to http://eslint.org/cla/
2. Checkout master on your computer, fetch upstream changes, and merge them.
3. Checkout a new branch.
4. As described in http://eslint.org/docs/developer-guide/contributing/pull-requests#step-2-make-your-changes, begin your commit message with Docs:

• README.md:* no-restricted-imports - restrict usage of specified node imports however the change here might be to delete node
• README.md:* no-restricted-modules - restrict usage of specified node modules
• callback-return.md: For example, in the case of an HTTP request, you may try to send HTTP headers more than once leading node.js to throw
• handle-callback-err.md:In node, a common pattern for dealing with asynchronous behavior is called the callback pattern.
• handle-callback-err.md:This rule expects that when you're using the callback pattern in node you'll handle the error and
• no-restricted-modules.md:Disallowing usage of specific node modules can be useful if you want to control the available methods, a developer can
• no-comma-dangle.md:If your code will not be run in IE8 or below (a NodeJS application, for example) and you'd prefer to allow trailing commas, turn this rule off.
• no-mixed-requires.md:In the Node.JS community it is often customary to separate the required modules from other variable declarations, sometimes also grouping them by their type. This rule helps you enforce this convention.
• no-mixed-requires.md:* The implementation is not aware of any local functions with the name require that may shadow Node's global require.
• no-mixed-requires.md:* Internally, the list of core modules is retrieved via require("repl")._builtinLibs. If you use different versions of Node.JS for ESLint and your application, the list of core modules for each version may be different.
• no-mixed-requires.md: The above mentioned _builtinLibs property became available in 0.8, for earlier versions a hardcoded list of module names is used as a fallback. If your version of Node is older than 0.6 that list may be inaccurate.
• no-restricted-imports.md:* Some imports might not make sense in a particular environment. For example, Node's fs module would not make sense in an environment that didn't have a file system.
• no-restricted-modules.md:# Disallow Node modules (no-restricted-modules)

[pedrottimark]

Indeed, import is a feature of ES6 and not limited to Node.js

How about omit ES6 and adjust the grammar so import can have code style and be singular.

[brettz9]

As far as your latter point, do you mean change the phrasing to something like "restrict usage of specified import statements"?

[pedrottimark]

Sorry, my mind stopped halfway through that thought. Looks like when no-restricted-imports derived from no-restricted-modules that imports replaced modules in the description. Both rules restrict certain modules, not the import or require itself. A thought (not necessarily a good one) would be to find a parallel grammar pattern for both descriptions so the restriction is clearly on modules and require or import fits fluently in code style.

[brettz9]

Can you propose a specific syntax for these? How about...

* [no-restricted-modules](no-restricted-modules.md) - restrict usage of specified modules when loaded by `require` statements
* [no-restricted-imports](no-restricted-imports.md) - restrict usage of specified modules when loaded by `import` statements

I assume you do not wish to change the rule name, no-restricted-imports, as well.

[pedrottimark]

The parallel grammar reads well.

Your question gave me a reason to improve my research skills 🤓 Only change I recommend is replace the word statements at the end of each description:

• …by require function [see https://nodejs.org/api/globals.html#globals_require and https://nodejs.org/api/modules.html#modules_addenda_package_manager_tips]
• …by import declaration [see http://www.ecma-international.org/ecma-262/6.0/index.html#sec-imports and https://github.com/eslint/eslint/blob/master/lib/rules/no-restricted-imports.js#L22]

For your info, making the description of each rule in the list consistent with the main heading in its page will probably happen in several weeks, so you do not need to worry about that now.

[brettz9]

I'm not sure how they're not consistent with the main heading, but you mention not to worry about that, so I won't. :)

Also, hopefully before you guys make further changes to the organization of the README, I'm just submitting these minor (and perhaps less controversial) fixes first, since I also have a PR ready which I plan to submit which gets rid of the environment sections entirely (putting the environment info in parentheses to allow users of the README to proceed through the page on a sequence based primarily on the cost-benefit degree).

[brettz9]

I filed issue #5505 regarding the proposal for a new directives issue.

And I've split this PR into #5507, #5508, and #5510 to handle most of the sub-tasks. I'm awaiting an answer to #5451 (comment) , however, before addressing the remaining item regarding the module rules having their description revised (and the ES6/Node moved appropriately).

Update: Now added PRs #5516 and #5517 to address the rest of this PR.

[pedrottimark]

@nzakas Can you please confirm whether it is okay to move two rules in the list?

• no-process-env from Best Practices to Node.js and CommonJS
• no-restricted-imports from Node.js and CommonJS to ECMAScript 6

[nzakas]

👍

[pedrottimark]

@brettz9 in case some context is helpful to you:

• “tactical” work-in-progress: Docs: WIP Guidelines for rule pages #5446 (if a share of it interests you, insert a comment to me)
• “strategic” discussion, which has been quiet while we work on the above: Improve information architecture of site archive-website#186

[brettz9]

Thank you, @pedrottimark . Separate PR's have been added for each subissue, so closing this one.

[pedrottimark]

You’re welcome!