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

ivangeorgiew · 2018-07-03T06:30:44Z

No description provided.

trotzig · 2018-07-03T07:04:22Z

lib/ImportStatement.js

-      return [this.path];
-    }
-    return [this.defaultImport || '', ...this.localNames()];
+    return [this.defaultImport !== 'React', this.path];


Why do you want React to be on top?

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.

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

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

trotzig · 2018-07-03T07:05:20Z

lib/visitIdentifierNodes.js

@@ -53,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') ||


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

added tests

trotzig · 2018-07-03T07:08:02Z

lib/Watcher.js

-        if (removed.length) {
-          this.onFilesRemoved(removed);
-        }
+        findAllFiles(this.workingDirectory, this.excludes)


While this may fix an issue for you, it's also going to slow down the file watcher significantly.

This is an issue with everyone using export * from './somePlace'
I don't think there is any other way to do this.
Also please try it out - I don't notice any slowdown whatsoever.

This method is called every time a change is made to a file (either modified, deleted or added). findAllFiles will traverse the filesystem, which will be slow on any large project. I think the reason you're not noticing this is that it's being done in the background.

The findExports module already has code to handle export ... from declarations. Can you see if you can inject your logic there instead of here?

import-js/lib/findExports.js

Line 14 in a6a7728

if (node.type === 'ExportAllDeclaration') {

Say you have a files a.js, b.js, c.js.
The contents of a.js are const abc = someNewVar
The contents of b.js are export * from './c'
The contents of c.js are export someOldVar = 123
When you add export someNewVar = 321 in file c.js
and you try to add the import in a.js the only option will be c.js
since file b.js didn't change. This is an incorrect behavior.
We need to add b.js to the processQueue method in ModuleFinder.js
Since I don't know how to do that - I implemented the only solution I saw

Okay, thanks for explaining the use-case. Still, we can't afford traversing through all files on every file modification. I think the way we should solve this is to introduce some kind of dependency list, so that we know to update exports for b.js when c.js is modified.

trotzig · 2018-07-03T07:09:49Z

lib/Watcher.js

+            files.forEach(({ path, mtime }: Object) => {
+              // add files which contain the following:
+              // export * from 'file'
+              if (fs.readFileSync(path, 'utf8').match(/export\s+\*\s+from/) !== null) {


This should go in findExports.js instead of here.

trotzig · 2018-07-03T07:13:05Z

lib/findProjectRoot.js

+
+    if (isRoot) {
+      return directory;
+    }


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. 👍

ivangeorgiew · 2018-07-03T08:16:01Z

I added tests for isRoot and info in the README

trotzig · 2018-07-03T18:36:11Z

lib/__tests__/findUndefinedIdentifiers-test.js

+  const foo = { 'theValueOfMyConst': 123 };
+  const { [MY_CONST]: someAlias } = foo;
+  `))).toEqual(new Set(['MY_CONST']));
+});


trotzig · 2018-07-03T18:40:40Z

lib/Watcher.js

-        if (removed.length) {
-          this.onFilesRemoved(removed);
-        }
+        findAllFiles(this.workingDirectory, this.excludes)


Okay, thanks for explaining the use-case. Still, we can't afford traversing through all files on every file modification. I think the way we should solve this is to introduce some kind of dependency list, so that we know to update exports for b.js when c.js is modified.

ivangeorgiew · 2018-07-19T08:49:38Z

@trotzig I updated the PR, I don't think anything more has to be done. Can you please merge it now?

ivangeorgiew · 2018-07-19T08:59:09Z

@trotzig What is the point of watching JSON files for changes btw? You can't import from json files as far as I know.

trotzig · 2018-07-19T17:41:34Z

lib/Watcher.js

+        //       }
+        //     });
+        //   });
+


This commented-out code isn't adding anything at the moment. I suggest opening a separate issue explaining what change in behavior you'd like to see.

trotzig · 2018-07-19T17:51:32Z

lib/ImportStatement.js

-      return [this.path];
-    }
-    return [this.defaultImport || '', ...this.localNames()];
+    return [this.defaultImport !== 'React', this.path];


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

trotzig · 2018-07-19T17:55:43Z

lib/ExportsStorage.js

-          const filtered = files.filter(({ path: pathToFile, mtime }) => (
-            mtime !== mtimes[pathToFile]
+          const filtered = files.filter(({ path: pathToFile, mtime, exportsAll }) => (
+            mtime !== mtimes[pathToFile] || exportsAll


Revert the changes made to these two lines. The exportsAll option is not in use after you cleaned out that logic.

lencioni

Thanks for working on this PR!

lencioni · 2018-07-20T10:48:01Z

README.md

@@ -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:


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

lencioni · 2018-07-20T10:51:03Z

lib/ImportStatement.js

-    }
-    return [this.defaultImport || '', ...this.localNames()];
+  toNormalized(): Array<any> {
+    return [this.defaultImport !== 'React', this.path];


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.

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

ivangeorgiew · 2018-07-26T05:53:19Z

I believe I made all the changes
Can you merge now?

ivangeorgiew · 2018-07-26T09:13:52Z

@trotzig @lencioni added logic for initializing the module finding when using an "init" function>
the necessary change was made in vim-import-js as well: Galooshi/vim-import-js#43

ivangeorgiew · 2018-08-06T07:21:58Z

@trotzig @lencioni I am done with everything you asked and some more. Can you please merge already?

trotzig · 2018-08-13T09:17:25Z

lib/ImportStatement.js

-      return [this.path];
-    }
-    return [this.defaultImport || '', ...this.localNames()];
+    return [this.path];


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).

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.

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.

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.

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.

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.

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

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.

trotzig · 2018-08-13T09:17:37Z

lib/ImportStatements.js

@@ -243,7 +243,8 @@ export default class ImportStatements {
      importsArray,
      (importStatement: ImportStatement): boolean =>
        !importStatement.isParsedAndUntouched(),
-    );
+    ).reverse();


This will have to go too.

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

trotzig · 2018-08-13T09:21:19Z

Sorry for the delay here @ivangeorgiew, I just came back from vacation. 🍹

Ivan Georgiev added 10 commits November 30, 2017 12:09

append imports option

6246b9a

update

e713147

import VALUE from obj[VALUE]

b063c3f

proper sorting

3b4e10e

add computed keys in aliased desctructured constants

1a7ee44

Merge branch 'master' of https://github.com/Galooshi/import-js

2c2f69e

added logic for checking files with export all from

0fd3a6d

specify project root

e3a4b11

fixed the tests

52f9e0f

performance improvement

9d3567e

trotzig reviewed Jul 3, 2018

View reviewed changes

Ivan Georgiev added 2 commits July 3, 2018 10:43

added test for aliases in destructured objects

7bceb5b

added test for isRoot and info in the README

124a792

trotzig reviewed Jul 3, 2018

View reviewed changes

Ivan Georgiev added 2 commits July 19, 2018 11:28

Merge branch 'append-imports'

60fc13f

fixed the tests

f64afde

ivangeorgiew mentioned this pull request Jul 19, 2018

append imports option #473

Closed

removed the export all logic for now

89f5e4b

trotzig requested changes Jul 19, 2018

View reviewed changes

removed the dead code for exports all

6b8657c

lencioni reviewed Jul 20, 2018

View reviewed changes

requested updates

ed94946

initialize function

4689499

ivangeorgiew mentioned this pull request Jul 26, 2018

start import-js automatically when first opening a js file Galooshi/vim-import-js#43

Closed

ivangeorgiew changed the title ~~Useful changes for my setup which I think may be beneficial to others~~ Useful changes from my setup which I think may be beneficial to others Jul 31, 2018

trotzig requested changes Aug 13, 2018

View reviewed changes

trotzig closed this Sep 10, 2018

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

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

Conversation

ivangeorgiew commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivangeorgiew commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivangeorgiew commented Jul 19, 2018

ivangeorgiew commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lencioni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivangeorgiew commented Jul 26, 2018

ivangeorgiew commented Jul 26, 2018 • edited Loading

ivangeorgiew commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trotzig commented Aug 13, 2018

ivangeorgiew commented Jul 26, 2018 •

edited

Loading