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

Useful changes from my setup which I think may be beneficial to others #506

Closed
wants to merge 18 commits into from
Closed
12 changes: 12 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,18 @@ Save this file in the root folder of your project (e.g. where the package.json
file is found). You can also save it to the user home directory if you want to
share a global config between different projects.

If you have a project inside a project with both containing a `package.json` file,
you can specify that the subproject is not root like this:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because most of the configuration for import JS is in the import JS configuration file, I think it would be helpful to make it clearer that this snippet is showing package.json. Maybe all we need to do is add "...not root like this in package.json:" or something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

```json
{
"name": "some-name",
"import-js": {
"isRoot": false
}
}
```
Note: the default is `isRoot: true`

The following configuration options are supported.

- [`aliases`](#aliases)
Expand Down
1 change: 0 additions & 1 deletion lib/ExportsStorage.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import sqlite3 from 'sqlite3';

import lastUpdate from './lastUpdate';

const MAX_CHUNK_SIZE = 100;
Expand Down
7 changes: 2 additions & 5 deletions lib/ImportStatement.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,8 @@ export default class ImportStatement {
/**
* @return {Array} an array that can be used in `sort` and `uniq`
*/
toNormalized(): Array<string> {
if (!this.defaultImport && !this.hasNamedImports() && this.hasSideEffects) {
return [this.path];
}
return [this.defaultImport || '', ...this.localNames()];
toNormalized(): Array<any> {
return [this.defaultImport !== 'React', this.path];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you want React to be on top?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this is the standard everywhere React is used. This way it won't interfere with other people on your project importing themselves (React is the first thing they import). You will omit unnecessary changes to a file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand. I haven't come across any project where React has to be at the top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I was wrong - it is only standard in the firm I work in. Therefore I left it to [this.path] only.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you help me understand why this change is made (including the type change)? It seems weird to have React hard-coded in this project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted the change. Now it is only [this.path]

}

localNames(): Array<string> {
Expand Down
7 changes: 4 additions & 3 deletions lib/ImportStatements.js
Original file line number Diff line number Diff line change
Expand Up @@ -243,14 +243,15 @@ export default class ImportStatements {
importsArray,
(importStatement: ImportStatement): boolean =>
!importStatement.isParsedAndUntouched(),
);
).reverse();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will have to go too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#448
This is one of the issues you have commented in the file above.
Look at the bottom.


result = flattenDeep(result);

if (this.config.get('sortImports')) {
result = sortBy(result, (is: ImportStatement): Array<string> => is.toNormalized());
result = sortBy(result, (is: ImportStatement): Array<any> => is.toNormalized());
}

result = uniqBy(result, (is: ImportStatement): Array<string> =>
result = uniqBy(result, (is: ImportStatement): Array<any> =>
is.toNormalized());

if (!this.config.get('groupImports')) {
Expand Down
2 changes: 2 additions & 0 deletions lib/Watcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ export default class Watcher {

const added = [];
const removed = [];

resp.files.forEach((file: Object) => {
const normalizedPath = normalizePath(
file.name,
Expand All @@ -88,6 +89,7 @@ export default class Watcher {
removed.push({ path: normalizedPath });
}
});

if (added.length) {
this.onFilesAdded(added);
}
Expand Down
32 changes: 16 additions & 16 deletions lib/__tests__/Importer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -496,10 +496,10 @@ foo
`.trim();

expect(subject()).toEqual(`
import foo from './bar/foo';
import bar from './bar';

import baz from './bar/baz';
import foo from './bar/foo';

foo
`.trim());
Expand All @@ -516,11 +516,11 @@ foo
`.trim();

expect(subject()).toEqual(`
import foo from './bar/foo';
import bar from './bar';

// Comment
import baz from './bar/baz';
import foo from './bar/foo';

foo
`.trim());
Expand All @@ -543,9 +543,9 @@ foo
// Comment A

// Comment B
import foo from './bar/foo';
import bar from './bar';
import baz from './bar/baz';
import foo from './bar/foo';

foo
`.trim());
Expand All @@ -563,12 +563,12 @@ foo
`.trim();

expect(subject()).toEqual(`
import foo from './bar/foo';
import bar from './bar';

// Comment A
// Comment B
import baz from './bar/baz';
import foo from './bar/foo';

foo
`.trim());
Expand All @@ -585,11 +585,11 @@ foo
`.trim();

expect(subject()).toEqual(`
import foo from './bar/foo';
import bar from './bar';

/* Comment */
import baz from './bar/baz';
import foo from './bar/foo';

foo
`.trim());
Expand All @@ -608,13 +608,13 @@ foo
`.trim();

expect(subject()).toEqual(`
import foo from './bar/foo';
import bar from './bar';

/* Comment
* 123
*/
import baz from './bar/baz';
import foo from './bar/foo';

foo
`.trim());
Expand Down Expand Up @@ -829,8 +829,8 @@ foo

it('adds the import and sorts the entire list', () => {
expect(subject()).toEqual(`
import bar from './foo/bar';
import foo from './bar/foo';
import bar from './foo/bar';
import zoo from './foo/zoo';

foo
Expand All @@ -852,10 +852,10 @@ foo

it('adds the import and sorts the entire list with groups', () => {
expect(subject()).toEqual(`
import bar from './foo/bar';
import foo from './bar/foo';
import * as starBar from './star/bar';
import bar from './foo/bar';
import zoo from './foo/zoo'
import * as starBar from './star/bar';

const sko = customImportFunction('./sko');

Expand All @@ -870,11 +870,11 @@ foo

it('adds the import and sorts all of them', () => {
expect(subject()).toEqual(`
import bar from './foo/bar';
import foo from './bar/foo';
import bar from './foo/bar';
import zoo from './foo/zoo'
const sko = customImportFunction('./sko');
import * as starBar from './star/bar';
import zoo from './foo/zoo'

foo
`.trim());
Expand Down Expand Up @@ -975,8 +975,8 @@ foo

it('adds the import, compacts, and sorts the entire list', () => {
expect(subject()).toEqual(`
import bar from './foo/bar';
import foo from './bar/foo';
import bar from './foo/bar';
import zoo from './foo/zoo';

foo
Expand All @@ -1002,9 +1002,9 @@ foo

it('compacts the list', () => {
expect(subject()).toEqual(`
import bar from './foo/bar';
import foo from './bar/foo';
import frodo from './bar/frodo';
import bar from './foo/bar';
import zoo from './foo/zoo';

foo
Expand Down Expand Up @@ -1578,8 +1578,8 @@ memoize
expect(subject()).toEqual(`
const bar = require('foo/bar');

var { memoize } = require('underscore');
var { xyz } = require('alphabet');
var { memoize } = require('underscore');

memoize
`.trim());
Expand Down Expand Up @@ -1700,8 +1700,8 @@ memoize

it('places the import at the right place', () => {
expect(subject()).toEqual(`
import { memoize } from 'underscore';
import { xyz } from 'alphabet';
import { memoize } from 'underscore';

import bar from 'foo/bar';

Expand Down Expand Up @@ -2976,8 +2976,8 @@ bar

it('sorts imports', () => {
expect(subject()).toEqual(`
import bar, { foo } from 'bar';
import baz from '../baz';
import bar, { foo } from 'bar';

bar
`.trim());
Expand Down
8 changes: 8 additions & 0 deletions lib/__tests__/findProjectRoot-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@ it('finds the right folders', () => {
.toEqual(path.join(path.resolve('/'), 'foo', 'bar'));
});

it('isRoot specification should work', () => {
// FileUtils.__setFile(path.join(process.cwd(), '.importjs.js'), {
fs.__setFile(path.join(path.resolve('/'), 'foo', 'package.json'));
fs.__setFile(path.join(path.resolve('/'), 'foo', 'bar', 'package.json'), '{ "import-js": { "isRoot": false } }');
expect(findProjectRoot(path.join(path.resolve('/'), 'foo', 'bar', 'baz.js')))
.toEqual(path.join(path.resolve('/'), 'foo'));
});

it('treats package folders as roots', () => {
fs.__setFile(path.join(path.resolve('/'), 'foo', 'package.json'));
fs.__setFile(path.join(path.resolve('/'), 'foo', 'node_modules', 'bar', 'package.json'));
Expand Down
7 changes: 7 additions & 0 deletions lib/__tests__/findUndefinedIdentifiers-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,13 @@ it('knows about dynamic keys', () => {
`))).toEqual(new Set(['bar']));
});

it('knows about aliases in destructured objects', () => {
expect(findUndefinedIdentifiers(parse(`
const foo = { 'theValueOfMyConst': 123 };
const { [MY_CONST]: someAlias } = foo;
`))).toEqual(new Set(['MY_CONST']));
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice.


it('knows about jsx', () => {
expect(findUndefinedIdentifiers(parse(`
export default <FooBar foo={bar} />;
Expand Down
8 changes: 7 additions & 1 deletion lib/findProjectRoot.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import fs from 'fs';
import path from 'path';
import FileUtils from './FileUtils';

const isWinDriveRoot = /^[A-Z]:\\$/;

Expand All @@ -11,7 +12,12 @@ function findRecursive(directory) {
const pathToPackageJson = path.join(directory, 'package.json');

if (fs.existsSync(pathToPackageJson)) {
return directory;
const packageDescription = FileUtils.readJsonFile(pathToPackageJson);
const { isRoot = true } = Object(packageDescription)['import-js'] || {};

if (isRoot) {
return directory;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks useful, and I'm assuming it's a continuation of #462? An update to findProjectRoot-test.js as well as an entry in the README and I think this is good to go. 👍

}

return findRecursive(path.dirname(directory));
Expand Down
5 changes: 2 additions & 3 deletions lib/visitIdentifierNodes.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
const KEYS_USED_FOR_ASSIGNMENT = new Set(['id', 'imported', 'local', 'params']);
const KEYS_USED_IN_REFERENCE_TO_OBJECTS = new Set(['property']);

function normalizeNode(node, context) {
const { key, parent } = context;
Expand Down Expand Up @@ -54,7 +53,7 @@ function normalizeNode(node, context) {
}

const isAssignment = KEYS_USED_FOR_ASSIGNMENT.has(key) ||
(key === 'key' && parent.parent.type === 'ObjectPattern') ||
(key === 'key' && !parent.computed && parent.parent.type === 'ObjectPattern') ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you come up with a test case demonstrating what this is supposed to fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added tests

(key === 'left' && parent.type === 'AssignmentPattern') ||
(key === 'elements' && parent.type === 'ArrayPattern') ||
(key === 'value' &&
Expand All @@ -64,7 +63,7 @@ function normalizeNode(node, context) {
context.definedInScope.add(node.name);
}

const isReference = KEYS_USED_IN_REFERENCE_TO_OBJECTS.has(key) ||
const isReference = (key === 'property' && !parent.computed && parent.parent.type !== 'ObjectPattern') ||
(key === 'key' && !parent.computed && parent.parent.type !== 'ObjectPattern');

return {
Expand Down
Loading