cvisco / eslint-plugin-requirejs Goto Github PK

For parity with no-invalid-define rule

A rule where something like this would fail:

Define(['dep1', 'dep2'], function(dep1) {
    //...
});

I've seen this a few times in our code base as an artifact of somebody messing up when removing a dependency.

I think the rule to control line breaks for define statements would be nice. I know I always struggled with this when I used requireJS. I adopted the rule that if define array had more then 3 items, then I write each item in array as well as in a callback function on the separate line, as such:

define([
  'Backbone',
  'underscore',
  'jquery',
  'someother'
], function(
  'Backbone',
  '_',
  '$',
  'someother'
) {
  ...
});

But if there are 3 or less items, then they can be written on a single line:

define(['Backbone', 'underscore', 'jquery'], function(Backbone, _, $) {
  ...
});

Number of max-params should probably be configurable.

The fixer for one-dependency-per-line uses LF unconditionally. In order to support CRLF repositories, an option should be added to support CRLF (e.g., { lineEnding: "\r\n" }).

Without this, people using auto-fix on this rule who also have the core ESLint linebreak-style rule set to Windows will generate errors in the latter rule when applying fixes from this rule.

Incidentally, I tried to get this sort of thing (duplicated rule configuration of CRLF and similar) handled in a more general "repository settings" section of ESLint configuration, but it got shot down, so we're stuck with the duplication.

Version: master

Sample code:

var MyGlobal = createMyGlobal();

Expected result: Error on the variable declaration line for not being in a define

Actual result: No error

Looking at the rule, it seems that this is due to checking for ExpressionStatement nodes that are not define calls. But the variable declaration statement is VariableDeclaration, not ExpressionStatement. You might want to instead report on any (non-comment?) node that is not both an ExpressionStatement and a define call.

The package.json file in this repository does not have a "files" key. The result is that when installing this package as a devDependency, unit tests and documentation (and some other files) are included when they don't need to be.

It's better to just have the minimum files needed to actually run the rules (probably lib/, index.js).

The following trivial example results in an error, and should not:

define(function (require, exports, module) {

    function addOne(thing) {
        return thing + 1; // flagged as error
    }

    module.exports = {
        addOne: addOne
    };
});

This will make configuration of rules easier and result in cleaner documentation. In addition to the no-invalid-define case, new rules should be:

• no-object-define
• no-function-define
• no-amd-define
• no-named define
• no-commonjs-wrapper

Just wanted to put this out there, are there any plans for making one-dependency-per-line fixable, so that it can be run with --fix and the dependencies are automatically formatted?

http://eslint.org/docs/user-guide/command-line-interface.html#fix

Make sure exports and/or module is provided as dependency if using it inside the module definition.

Make sure require is declared as a dependency if using relative dependency paths inside a module

The no-invalid-require rule has some comments about wanting to flag only obviously incorrect cases and not flagging expressions that could evaluate to correct cases. However, the checks for FunctionExpression nodes (as last arguments of 2- or 3-argument forms of require()) are stricter than that, and are causing some problems in my application.

Here are the two examples I'm working with:

function requireCultures(callback) {
    var culturePaths = [];
    // ...
    require(culturePaths, callback);
}

require([translationPath], _.bind(function(translation) {
    // ...
}, this));

In the first example, I have only an identifier, which I trust to be a function callback. In the second, I am using Underscore.js and I can affirm that this expression will return a function (and it even takes a FunctionExpression I might otherwise have passed directly to require), but the argument is a CallExpression in the AST, not a FunctionExpression.

I think this part of the rule should be similar to the rest and should flag against ObjectExpression, arrays, or basic literals, but should assume that identifiers or CallExpression nodes could resolve to a function callback.

Disallow use of module.exports when using Simplified CommonJS Wrapper

It would be great to have a rule which will check the required paths and will enforce user to have it in alphabetical order.

This will be considered as warning ('foo/baz' should be AFTER 'foo/bar'):

require([
'foo/baz',
'foo/bar'
], function (
baz,
bat) {
});

This will NOT be considered as warning ('foo/baz' IS AFTER 'foo/bar'):

require([
'foo/bat',
'foo/baz'
], function (
bat,
baz) {
});

Noone cares about order of function params (only path should be checked).

Thanks!

Disallow use of exports when using Simplified CommonJS wrapper

Right now, in order to add new test fixtures, some manual work must be done (adding to test/fixtures/index.js, etc.). I am wondering if the "requireindex" package might be able to do some of this dynamically.

seems to comparing against full path, not just filename

In mixed codebases, where you might have node scripts, alongside amd modules for the browser, it would be beneficial to only enable the plugin's rules if the environment for a given file is actually amd. That is it has require and define as globals

Rules have been sometimes warning on non-define calls. For example, in no-object-define:

// This warns, as it should
define('name', {});

// This also warns, and it shouldn't:
foo('name', {});

This applies to all current rules.

Disallow defining more than one module per javascript source file

Prevent assignment to require. RequireJS provides a method of passing configuration options by assigning an object to require before require.js is loaded (see http://requirejs.org/docs/api.html#config) This can break alternate module loaders such as almond.js (https://github.com/jrburke/almond#restrictions)

Only allow paths in require calls

this pattern causes eslint to crash:

var lib = 'some/lib';
require([lib], function () { /* ... */ });

Prevent usage of require.toUrl() and require.nameToUrl(). These aren't supported by some alternate module loaders such as almond.js

I think it's because the fixtures are being checked out as CRLF but the fixer code uses \n only.

The solution to this might be a .gitattributes file that basically declares everything in the repository must be LF (and I'm fairly sure my Windows box could deal with it).

Named (or aliased) define calls may also be a simple function or object-style define as well

Test case:

require(that._configModel.getDependenciesPath(), function (product) {
    that._dependenciesLoaded(product);
});

Expected: No warning (how can it know what the function call will return?).

Actual: "Too many parameters in require callback (expected 0, found 1)."

Presumably it should just bail out without warning if the first argument to define/require/requirejs is not an ArrayExpression.

If this is accepted as a bug, I can work on a fix.

Sort rules in README according to "errors" and "style issues"

Paths in require do not need a .js extension if they are JavaScript files

None of the makefile tasks work on Windows by default. Some of the tasks are specified using shell commands that work in a Bash-compatible shell; ideally, these would be transformed to use Node.js paths and things like that, which would help normalize OS inconsistencies.

The no-multiple-define rule works by iterating through all tokens in a program and reporting only once at the very end if the total number of define identifiers is greater than 1. This results in one message at the beginning of the file. I think the rule could be more useful if it reported all violating nodes (after the first one), so that line and column information is accurate.

In addition, the rule should not need to iterate over all tokens, only CallExpressions (after all, using the identifier define does not always imply a module definition, such as typeof define === "function"). By changing the rule to run its checks on CallExpression nodes, that should make the rule a bit faster. The rule can still keep track of define calls by closing over the count variable.

I'm happy to write a PR if you accept this issue.

If this is accepted, I can work on a pull request for adding /* istanbul ignore else */ or /* istanbul ignore next */ directives for some of the areas in rules identified by Istanbul as lacking coverage. (I'm fairly sure all the ones in the rules themselves can be marked with ignore directives. The ones in lib/util.js I'm not so sure of.)

Disallow require statements executed conditionally, for example inside of an if statement or ternary

Require that all files be wrapped in a define

The rule should make sure that when defining a module in the AMD form of define, the function should have the same length (arity) as the array.
Example:
valid:

define(['a', 'b'], function(a, b) { 
  // module code...
});

invalid:

define(['a', 'b'], function(a) {
  // module code that doesn't use b
});

define(['a', 'b'], function(a, b, c) {
 // c is undefined
});

I know in some cases requiring a dependency is used for side effects, but I think in most cases it's just a bug, so the edge cases can disable the rule for the line.

Prevent direct assignment to exports object, this is probably an error. Assignment should only be done to module.exports. This should be on by default.

Prevent returning a value from modules defined with the Simplified CommonJS wrapper. This encourages use of module.exports or exports instead.

This would allow storing and editing example code as actual JavaScript files.

Change peerDependency version for eslint to work with other versions besides 0.17