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

Proposal: `default-import-match-filename` #325

Open
gajus opened this issue May 9, 2016 · 14 comments · May be fixed by #1476
Open

Proposal: `default-import-match-filename` #325

gajus opened this issue May 9, 2016 · 14 comments · May be fixed by #1476

Comments

@gajus
Copy link
Contributor

@gajus gajus commented May 9, 2016

I often see things such as:

import parseDate from './types/date';
import float from './types/parseFloat';

The disparity between the default import name (parseDate) and file name (date) often indicates that one or the other is named inappropriately. In the first case, thats the file name. In the second case, thats the local variable name.

Valid code:

import parseFloat from './types/parseFloat';
import parseDate from './types/parseDate';

Invalid code:

import parseDate from './types/date';
import float from './types/parseFloat';

There is no way to enforce a right name, e.g. user could just as well end up with:

import date from './types/date';
import float from './types/float';

However, I would argue that these issues are easy to notice in the code. Having this rule would prevent lazy developers from using a better variable name in a local file and ignore the need to rename the file and other references.

Furthermore, this enables easier code inspection (because it is easy to recognise what a variable represents when it is being used consistently between many files).

@benmosher

This comment has been minimized.

Copy link
Owner

@benmosher benmosher commented May 9, 2016

Makes sense to me. Probably needs a little more detail/scope to build out.

How would you propose packages be handled? I.E.

import React from 'react'

is idiomatic, but not sure how one would divine this from just the import path.

@gajus

This comment has been minimized.

Copy link
Contributor Author

@gajus gajus commented May 9, 2016

I would propose to have a separate rule for overseeing module imports and separate rule for importing files, i.e. default-import-match-module and default-import-match-filename. I will leave the question of react case to whoever becomes a proponent of the default-import-match-module rule.

@benmosher

This comment has been minimized.

Copy link
Owner

@benmosher benmosher commented May 9, 2016

Got it, so this rule's scope would be only internal relative imports, as you've described?

@jfmengels

This comment has been minimized.

Copy link
Collaborator

@jfmengels jfmengels commented May 9, 2016

We've had a similar discussion on eslint-plugin-xo sindresorhus/eslint-plugin-unicorn#6, though we only spoke about modules (but I think they should probably be considered the same).

I have trouble seeing this as something that can be generalized enough. For example, how would you enforce / generalize the following?

import parseFloat from './parse/float';
import parseDate from './parse/date';
import formatFloat from './format/float';
import formatDate from './format/date';

or

import closeToFoo from './foo';

function foo(...) { ... }

export foo;

cc @jamestalmage @sindresorhus

@benmosher

This comment has been minimized.

Copy link
Owner

@benmosher benmosher commented May 9, 2016

I would think that such a folder layout simply couldn't use the rule that @gajus has described.

It would be workable as he has described if it stays as simple as "default import identifiers should match an imported file's name".

@gajus

This comment has been minimized.

Copy link
Contributor Author

@gajus gajus commented May 9, 2016

First of all, if you have:

import parseFloat from './parse/float';
import parseDate from './parse/date';
import formatFloat from './format/float';
import formatDate from './format/date';

This is specifically the design flaw that I am describing.

What is the reason you are naming your variables different than the files? In this specific context, it is because variable name makes more sense in the code than simply "float" or "date". Then what is the reason you are naming a file containing "parseFloat" function as "float"? It makes no sense. When considering a file name, file path should have no weight. With this rule, you would be forced to restructure your code as:

import parseFloat from './parse/parseFloat';
import parseDate from './parse/parseDate';
import formatFloat from './format/formatFloat';
import formatDate from './format/formatDate';

(or choose not to use the rule).

@jfmengels

This comment has been minimized.

Copy link
Collaborator

@jfmengels jfmengels commented May 9, 2016

When considering file name, file path should have no weight

Hm, I'd like to disagree. I don't see why the file path should have no weight. If I have my float handler in a parse/ folder, then I'm just repeating myself, and I don't see the point. I also often have a folder with an index.js file (like foo/bar/index.js), so that I can just do import bar from './foo/bar'; and "hide" the internal workings in other files. It's not per se in conflict with the rules, just pointing out that I need the file path to infer what foo/bar/index.js does, while being a valid use IMO. This pattern has worked fine for me thus far.
(if you guys feel like I'm taking the conversion somewhere it shouldn't, I'll be happy to discuss this somewhere else).

(or choose not to use the rule)

That is of course an acceptable solution ;) I have no issue with this issue being in the repo at all. I just have a concern that if you generalize this to external modules (it is similar, but I guess that it is done for different purposes here, so one could argue one way or the other IMO), then you'd have issues all over the place for handling it one way or another, have exceptions, etc., and that could be a huge burden on the maintainer.

@gajus

This comment has been minimized.

Copy link
Contributor Author

@gajus gajus commented May 9, 2016

Hm, I'd like to disagree. I don't see why the file path should have no weight. If I have my float handler in a parse/ folder, then I'm just repeating myself, and I don't see the point. I also often have a folder with an index.js file (like foo/bar/index.js), so that I can just do import bar from './foo/bar';

I can think of many examples. However, style is one of those things that it is easy to find arguments in both directions.

To quickly summarize my position:

  1. You want to name files the same way you will name the variable that will represent the file contents. The reason: it will promote consistent name of a resource throughout the code base.

  2. A path of the file will not always be visible. Consider example:

    ./formatters/hour.js
    ./formatters/minute.js
    ./formatters/time.js
    

    Now in ./time.js you want to import ./hour.js and ./minute.js.

    You cannot tell what ./hour and ./minute only by scanning the code (such is often the case when reviewing code fragments in PRs)

There are a few more arguments (such as promoting code re-usability), though this is getting off-topic.

This rule is opt-in and it will benefit developers who prefer explicit code naming conventions.

@mizchi

This comment has been minimized.

Copy link

@mizchi mizchi commented Feb 18, 2017

I think we need default-export-match-filename too.

Example

// MyComponent.js
// pass
export default function MyComponent() { return ... }
// fail
export default function NotMyComponent() { return ... }
@ljqx

This comment has been minimized.

Copy link
Contributor

@ljqx ljqx commented Feb 20, 2017

I think this rule (both export & import) can be configured by

  1. what to match
  • base file name
  • full path from project root (module id?)
  1. how to match
  • exact match (as Airbnb style)
  • filename/module id kebab case (friendly to Windows users, consistent with npm)
  • ...
  1. how to treat index.js
  • no special handling
  • the folder name is used as base file name or full path end

Then we can fit all requirements from different projects above, and leave the option to users.

For

import parseFloat from './parse/float';
import parseDate from './parse/date';
import formatFloat from './format/float';
import formatDate from './format/date';

It's valid for "full path" match, but not "base file name" match.

@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented Feb 20, 2017

I'm not sure about your latter example - is parseFloat valid if the path is ./a/b/c/parse/float?

I think the rule should only ever use the base filename, or the base dirname if it's an index (the latter could be configurable, but should be on by default).

The rule would need to handle the most common cases; it's not practical for it to attempt to cover every conceivable pattern.

@ljqx

This comment has been minimized.

Copy link
Contributor

@ljqx ljqx commented Feb 22, 2017

@ljharb
parseFloat is not valid when the path is ./a/b/c/parse/float for either “module specifier" match or "base filename" match. There will be only one parseFloat for the whole project, I think it's ok that it's at the top level of project as parseFloat.js (base filename or module specifier) or parse/float.js(module specifier) for grouping.

Matching base filename is not always good, especially for big project which has complicated features which is grouped by folders. As an example, there is one file named feature/subFeature/subSubFeature/Editor, we want it have a good name for readability/debuggability, then should we change the file to feature/subFeature/subSubFeature/FeatureSubFeatureSubSubFeatureEditor? Why duplicate the path again? That's same for parse/float too: parseFloat shows up with parse/float when importing import parseFloat from 'parse/float', later parseFloat is self-describing by its name, every code piece is readable, and no duplicate in path.

And, ./a/b/c/parse/float is not covered here :).

There maybe another configuration for all exceptions like import _ from 'lodash'.

@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented Feb 22, 2017

Any argument for parse/float being parseFloat also argues for feature/parse/float being featureParseFloat.

@keyboard-clacker

This comment has been minimized.

Copy link

@keyboard-clacker keyboard-clacker commented Sep 13, 2018

Is there any movement on this? Or was it dropped :( I was looking around to see if this was a thing and stumbled upon this thread, would be awesome if even the rule was "imported value should have the same name as the export if the export is named"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

7 participants
You can’t perform that action at this time.