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

Update: add named imports and exports for object-curly-newline #9876

Merged
Merged
50 changes: 48 additions & 2 deletions docs/rules/object-curly-newline.md
Expand Up @@ -19,19 +19,23 @@ Or an object option:
* `"minProperties"` requires line breaks if the number of properties is at least the given integer. By default, an error will also be reported if an object contains linebreaks and has fewer properties than the given integer. However, the second behavior is disabled if the `consistent` option is set to `true`
* `"consistent": true` requires that either both curly braces, or neither, directly enclose newlines. Note that enabling this option will also change the behavior of the `minProperties` option. (See `minProperties` above for more information)

You can specify different options for object literals and destructuring assignments:
You can specify different options for object literals, destructuring assignments, named imports and exports:
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Could you please add "and" before "named imports and exports"? Thanks!


```json
{
"object-curly-newline": ["error", {
"ObjectExpression": "always",
"ObjectPattern": { "multiline": true }
"ObjectPattern": { "multiline": true },
"ImportDeclaration": "never",
"ExportDeclaration": { "multiline": true, "minProperties": 3 }
}]
}
```

* `"ObjectExpression"` configuration for object literals
* `"ObjectPattern"` configuration for object patterns of destructuring assignments
* `"ImportDeclaration"` configuration for named imports
* `"ExportDeclaration"` configuration for named exports

### always

Expand Down Expand Up @@ -463,6 +467,48 @@ let {k = function() {
}} = obj;
```

### ImportDeclaration and ExportDeclaration

Examples of **incorrect** code for this rule with the `{ "ImportDeclaration": "always", "ExportDeclaration": "never" }` options:

```js
/*eslint object-curly-newline: ["error", { "ImportDeclaration": "always", "ExportDeclaration": "never" }]*/
/*eslint-env es6*/

import {foo, bar} from 'foo-bar';
import {foo as f, bar} from 'foo-bar';
import {foo,
bar} from 'foo-bar';

export {
foo,
bar
};
export {
foo as f,
bar
} from 'foo-bar';
```

Examples of **correct** code for this rule with the `{ "ImportDeclaration": "always", "ExportDeclaration": "never" }` options:

```js
/*eslint object-curly-newline: ["error", { "ImportDeclaration": "always", "ExportDeclaration": "never" }]*/
/*eslint-env es6*/

import {
foo,
bar
} from 'foo-bar';
import {
foo as f,
bar
} from 'foo-bar';

export { foo, bar } from 'foo-bar';
export { foo as f, bar } from 'foo-bar';
```

## Compatibility

* **JSCS**: [requirePaddingNewLinesInObjects](http://jscs.info/rule/requirePaddingNewLinesInObjects) and [disallowPaddingNewLinesInObjects](http://jscs.info/rule/disallowPaddingNewLinesInObjects)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a JSCS rule we are now compatible with for padding new lines for import/export specifiers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There doesn't seem to be any JSCS rule regarding new lines and import/export specifiers. Only ones for spaces in the import braces.

requireSpacesInsideImportedObjectBraces
disallowSpacesInsideImportedObjectBraces

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking!

Expand Down
80 changes: 62 additions & 18 deletions lib/rules/object-curly-newline.js
Expand Up @@ -73,19 +73,32 @@ function normalizeOptionValue(value) {
* Normalizes a given option value.
*
* @param {string|Object|undefined} options - An option value to parse.
* @returns {{ObjectExpression: {multiline: boolean, minProperties: number}, ObjectPattern: {multiline: boolean, minProperties: number}}} Normalized option object.
* @returns {{
* ObjectExpression: {multiline: boolean, minProperties: number, consistent: boolean},
* ObjectPattern: {multiline: boolean, minProperties: number, consistent: boolean},
* ImportDeclaration: {multiline: boolean, minProperties: number, consistent: boolean},
* ExportNamedDeclaration : {multiline: boolean, minProperties: number, consistent: boolean}
* }} Normalized option object.
*/
function normalizeOptions(options) {
if (options && (options.ObjectExpression || options.ObjectPattern)) {
if (options &&
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if this could be replaced with something like lodash.isPlainObject(options)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering that the options can take different forms, more checks must be made for more node specific options.

I've made changes in 75bea25 to make the code more readable.

Explanation:

"never"
"always"

{ "multiline": true }
{ "minProperties": 2 }
{ "consistent": true }

// want to identify this
{ "ObjectExpression": "always" }
{ "ObjectExpression": { "multiline": true } }
  1. find options that are objects.
  2. find the options that have a string or object as its value.

(
options.ObjectExpression ||
options.ObjectPattern ||
options.ImportDeclaration ||
options.ExportDeclaration
)) {
return {
ObjectExpression: normalizeOptionValue(options.ObjectExpression),
ObjectPattern: normalizeOptionValue(options.ObjectPattern)
ObjectPattern: normalizeOptionValue(options.ObjectPattern),
ImportDeclaration: normalizeOptionValue(options.ImportDeclaration),
ExportNamedDeclaration: normalizeOptionValue(options.ExportDeclaration)
};
}

const value = normalizeOptionValue(options);

return { ObjectExpression: value, ObjectPattern: value };
return { ObjectExpression: value, ObjectPattern: value, ImportDeclaration: value, ExportNamedDeclaration: value };
}

//------------------------------------------------------------------------------
Expand All @@ -109,7 +122,9 @@ module.exports = {
type: "object",
properties: {
ObjectExpression: OPTION_VALUE,
ObjectPattern: OPTION_VALUE
ObjectPattern: OPTION_VALUE,
ImportDeclaration: OPTION_VALUE,
ExportDeclaration: OPTION_VALUE
},
additionalProperties: false,
minProperties: 1
Expand All @@ -125,32 +140,59 @@ module.exports = {

/**
* Reports a given node if it violated this rule.
*
* @param {ASTNode} node - A node to check. This is an ObjectExpression node or an ObjectPattern node.
* @param {{multiline: boolean, minProperties: number}} options - An option object.
* @param {ASTNode} node - A node to check. This is an ObjectExpression, ObjectPattern, ImportDeclaration or ExportNamedDeclaration node.
* @param {{multiline: boolean, minProperties: number, consistent: boolean}} options - An option object.
* @returns {void}
*/
function check(node) {
const options = normalizedOptions[node.type];

if (
(node.type === "ImportDeclaration" &&
!node.specifiers.some(specifier => specifier.type === "ImportSpecifier")) ||
(node.type === "ExportNamedDeclaration" &&
!node.specifiers.some(specifier => specifier.type === "ExportSpecifier"))
) {
return;
}

const openBrace = sourceCode.getFirstToken(node, token => token.value === "{");

let closeBrace;

if (node.typeAnnotation) {
closeBrace = sourceCode.getTokenBefore(node.typeAnnotation);
} else {
} else if (node.type === "ObjectExpression" || node.type === "ObjectPattern") {
closeBrace = sourceCode.getLastToken(node);
} else {
closeBrace = sourceCode.getTokenAfter(node.specifiers.slice(-1)[0], token => token.value === "}");
Copy link
Member

Choose a reason for hiding this comment

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

Could we use sourceCode.getLastToken(node, token => token.value === "}") in both this code path and the one for ObjectExpression/ObjectPattern?

}

let first = sourceCode.getTokenAfter(openBrace, { includeComments: true });
let last = sourceCode.getTokenBefore(closeBrace, { includeComments: true });
const needsLinebreaks = (
node.properties.length >= options.minProperties ||
(
options.multiline &&
node.properties.length > 0 &&
first.loc.start.line !== last.loc.end.line
)
);

let needsLinebreaks;
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to extract this code into its own function to make things a little more readable.


if (node.type === "ObjectExpression" || node.type === "ObjectPattern") {
needsLinebreaks = (
node.properties.length >= options.minProperties ||
(
options.multiline &&
node.properties.length > 0 &&
first.loc.start.line !== last.loc.end.line
)
);
} else {
needsLinebreaks = (
node.specifiers.length >= options.minProperties ||
(
options.multiline &&
node.specifiers.length > 0 &&
first.loc.start.line !== last.loc.end.line
)
);
}

const hasCommentsFirstToken = astUtils.isCommentToken(first);
const hasCommentsLastToken = astUtils.isCommentToken(last);

Expand Down Expand Up @@ -244,7 +286,9 @@ module.exports = {

return {
ObjectExpression: check,
ObjectPattern: check
ObjectPattern: check,
ImportDeclaration: check,
ExportNamedDeclaration: check
};
}
};