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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
6246b9a
append imports option
Nov 30, 2017
e713147
update
Nov 30, 2017
b063c3f
import VALUE from obj[VALUE]
Apr 12, 2018
3b4e10e
proper sorting
Apr 12, 2018
1a7ee44
add computed keys in aliased desctructured constants
Apr 20, 2018
2c2f69e
Merge branch 'master' of https://github.com/Galooshi/import-js
May 31, 2018
0fd3a6d
added logic for checking files with export all from
Jun 1, 2018
e3a4b11
specify project root
Jun 9, 2018
52f9e0f
fixed the tests
Jun 10, 2018
9d3567e
performance improvement
Jun 19, 2018
7bceb5b
added test for aliases in destructured objects
Jul 3, 2018
124a792
added test for isRoot and info in the README
Jul 3, 2018
60fc13f
Merge branch 'append-imports'
Jul 19, 2018
f64afde
fixed the tests
Jul 19, 2018
89f5e4b
removed the export all logic for now
Jul 19, 2018
6b8657c
removed the dead code for exports all
Jul 20, 2018
ed94946
requested updates
Jul 23, 2018
4689499
initialize function
Jul 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions README.md
View file
Open in desktop
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 inside `package.json` like this:
```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
View file
Open in desktop
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
5 changes: 1 addition & 4 deletions lib/ImportStatement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,10 +149,7 @@ 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()];
return [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.

This change will modify sorting of imports. From experience, I know there will be heated discussions around this. Would you mind moving this out of the PR and pushing it as a separate PR? Or better yet, a PR adding support for better user-control over sorting (and grouping).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this sorting, the node_modules imports will be at the top with them being sorted by package name.
After them will be the rest of the imports which will also be sorted by filename.

I really don't see any other possible sorting as viable. Sorting by the imports themselves and not where they come from doesn't make any sense. This will produce many unnecessary/confusing merge conflicts. Example: instead of only removing a named export, the change will also include the movement of the imports for that file due to the sorting. The same scenario will be for appending named imports.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There has been plenty of discussion around sorting and grouping, and it's pretty clear that we need more flexibility:
#508
#454
#448
#284

I think the best thing we could do is to look at what the sort-imports eslint plugin does and make things configurable the same way.

I'm not going to argue much with what you think is the right order. What's important here is that we can either introduce a breaking change (that doesn't fix the problem) or keep the current sorting until we have a non-breaking and flexible solution. Introducing a breaking change on import sort order right now would mean introducing churn for every project that has been using import-js for a while.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two of those issues are things that I fixed myself - the empty line between imports being removed and the newly added imports being appended instead of prepended.
The other two issues are from people which want exactly the same sort order as I have currently implemented in this PR.
So I don't really see any conflicts of interest... Everyone wants the same thing.
Those issue links just highlight what I have told you in my previous comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I really don't see any other possible sorting as viable

Let me present you with a few possible sorting options that are all equally viable:

  • a-z on the variable name (the assignment)
  • a-z on the module name (the path/package name)

For groups, any combination of the following seems equally viable:

  • built-in packages
  • node_modules
  • non-js files
  • side-effect imports (those that have no assignment)
  • relative paths
  • absolute paths

The right thing here would be to make things configurable, so that you can choose any sorting and any grouping. If we make the default config produce the same sorting and grouping result as what we have today by default, I'll be happy to merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First option:

a-z on the variable name (the assignment)

This is not viable, because of I what I wrote in the first comment...
Since you haven't read it I will quote it here...

Sorting by the imports themselves and not where they come from doesn't make any sense. This will produce many unnecessary/confusing merge conflicts. Example: instead of only removing a named export, the change will also include the movement of the imports for that file due to the sorting. The same scenario will be for appending named imports.

Second option:

a-z on the module name (the path/package name)

This is what I have currently implemented...

For the grouping, it is very simple - exports from outside of the project, after which - local exports. I think everyone will find that as a reasonable default.

If you find any value in adding a custom sorting/grouping function that will benefit anyone (since I haven't seen one such person in the above mentioned issues by you),
then do that yourself. I have done enough work, I am happy with what I've got and I consider it to be the best default behavior for the project.

Copy link
Contributor

Choose a reason for hiding this comment

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

I currently have a few projects using the current default sorting and grouping rules and I agree that this shouldn't be changed until it's configurable. Otherwise I'll need to turn it off and manually sort and group again, or deal with PR churn for the foreseeable future. I'm sure there are others using the default behaviour that we don't hear from.

Because this is an area where it's down to preference, and an existing default has been in use for a fairly long time, it does not make sense to change the default and push a different preference on those already using the default behaviour.

Now @trotzig has not asked you to implement custom sorting configuration, only to keep the existing defaults so it doesn't create churn for other users.
I understand the frustration of having a PR held up when you feel that you have done a good job, but lets keep some perspective.

Also, you have done a great job here. As a user I always appreciate when someone makes an effort to improve OSS tools for the community

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current setup doesn't make sense for the reason I have pointed out above. That is why I changed it to something that does. You simply don't want the sorting to be changed because that is what you have used in existing projects. I consider this argument invalid. Keeping wrong code because it is what people have used so far is not correct. Please give me an argument that supports the claim that the current sorting is in any way better than the one I have implemented.
If you can't, someone else will have to implement these features listed here, because I got tired of this.

}

localNames(): Array<string> {
Expand Down
3 changes: 2 additions & 1 deletion lib/ImportStatements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,8 @@ 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')) {
Expand Down
2 changes: 2 additions & 0 deletions lib/Watcher.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
9 changes: 9 additions & 0 deletions lib/daemon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,15 @@ export default function daemon(parentPid, pathToLogFile) {
const workingDirectory = findProjectRoot(payload.pathToFile);
winston.debug(`Using ${workingDirectory} as project root for ${payload.pathToFile}`);

if (payload.command === 'init') {
initializeModuleFinder(workingDirectory).then(() => {
winston.debug(`ImportJS is initializing for ${workingDirectory}. Results will be more accurate in a few moments.`);
process.stdout.write('\n');
});

return;
}

const importer = new Importer(
payload.fileContent.split('\n'),
payload.pathToFile,
Expand Down
8 changes: 7 additions & 1 deletion lib/findProjectRoot.js
View file
Open in desktop
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;
}
}

return findRecursive(path.dirname(directory));
Expand Down
5 changes: 2 additions & 3 deletions lib/visitIdentifierNodes.js
View file
Open in desktop
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