Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Consider improving config options for import-name #452

Closed
JoshuaKGoldberg opened this issue Jul 5, 2018 · 6 comments
Closed

Consider improving config options for import-name #452

JoshuaKGoldberg opened this issue Jul 5, 2018 · 6 comments

Comments

@JoshuaKGoldberg
Copy link

Continuing discussion from #429: how do we allow full import names, such as "react-dom/server"?

/cc @massimonewsuk @levithomason

@JoshuaKGoldberg JoshuaKGoldberg added the Status: In Discussion Please continue discussing the proposed change before sending a pull request. label Jul 5, 2018
@levithomason
Copy link
Member

Since dependency names are unique, I would use those as keys in the config object. The values in the config object would be the allowed name it can be imported as.

@massimonewsuk
Copy link

massimonewsuk commented Jul 6, 2018

I agree with levithomason

Other things to consider (and possibly get feedback on) would be:

  • Should we allow multiple names for an import? i.e. "lodash": ["_", "lodash", "foo"]
    I don't think this is a great idea considering the whole purpose of this rule is to normalise the way people import things
  • Should we allow the user to still use the original name? i.e. "lodash: ["_"]", should the user still be allowed to do import lodash from "lodash"?
    Not sure about this one. But the above consideration should also be considered here. However, it's probably best to still allow the original name, especially when you consider the following types of scenarios:
{
    "sequelize-typescript": "sequelize"
}

Then in the code:

import sequelizeTypescript from "sequelize-typescript";
import sequelize from "sequelize";
sequelizeTypescript.configure(sequelize)

And elsewhere

import sequelize from "sequelize-typescript";

So, all in all, my personal vote is for what @levithomason suggested in the other issue, but to still also allow the default name of the module too so that we can deal with the above scenarios.

Edit: However, it might also be possible to deal with the above scenarios using tslint ignores. Maybe allowing the default name should be configurable?

@levithomason
Copy link
Member

Should we allow multiple names for an import?

I would also say no, as it goes against the heart of the rule which is to standardize import names.

Should we allow the user to still use the original name?

Also no, for the same reason as above.

I would lean toward making it very explicit with no special casing. In my experience, special cases tend to stack up and lead to complications whereas explicit and simple code is more maintainable.

One last idea I had while setting this up was about allowing pattern matching or someway to say "camelCase" as the import style. Specifically, I was mapping lots modules that started with the same prefix (e.g. gulp-* or babel-plugin-*) and wondering how you might match all modules by pattern and then allow camel case from there. I'm not sure if this is too much magic in the end.

@karol-majewski
Copy link

Please consider an option to ignore file extensions.

In this example:

import example from './example.svg';

the suggested name is exampleSvg. A similar thing happens to JSON files — TypeScript 2.9 brought extended support for importing JSON files, but the requirement of having example called exampleJson when it's imported leaves an uneasy feeling.

@sbuzonas
Copy link

sbuzonas commented Jul 26, 2018

One last idea I had while setting this up was about allowing pattern matching or someway to say "camelCase" as the import style. Specifically, I was mapping lots modules that started with the same prefix (e.g. gulp-* or babel-plugin-*) and wondering how you might match all modules by pattern and then allow camel case from there. I'm not sure if this is too much magic in the end.

Not too much magic, it's a common use case IMO.

I would lean toward making it very explicit with no special casing. In my experience, special cases tend to stack up and lead to complications whereas explicit and simple code is more maintainable.

I don't think these are special cases; I agree it should be explicit, but there is much to consider IRT import names and it should have robust configuration.

The way I see it, there are a few separate concerns being discussed here:

Renaming/Collisions

{
    "rename": {
        "server": "srv",
        "react-dom/server": "ReactDOMServer"
    }
}

I think it should accept the existing behavior and allow for longer path matches, choosing the longest match as the winner.

IRT, example code:

import sequelizeTypescript from "sequelize-typescript";
import sequelize from "sequelize";
sequelizeTypescript.configure(sequelize)

I think tslint ignore is satisfactory in this case.

Prefixes/Suffixes

{
    "ignore-prefix": [
        "babel-plugin-",
        "gulp-"
    ],
    "ignore-suffix": [
        ".json",
        ".svg",
        "-ts",
        "-typescript"
    ]
}

I found this discussion in the process of creating a feature request for the ./example.svg scenario. This should be clean enough to handle a number of different cases mentioned in this thread.

Edge Cases/Bulk Behavior

{
    "overrides": {
        "gulp-*": {},
        "sequelize-typescript": {},
    }
}

This configuration would have a pattern as the key and accept the entire rule schema as the value. I'm not sure how I feel about this one, but it was an idea to address the wildcard matching mentioned earlier. It can be pretty powerful in handling unknown corner cases and would pair nicely with something like an import-style directive that accepts things like "camelCase" and "pascalCase" to control the naming behavior.

Edit
One additional note. Couldn't decide if I should comment on #125 or add to this discussion. I was looking for a rule to match the import name with the export name. #125 is named exactly that, but was closed by the introduction of the import-name rule from #21.

I was thinking something like a tandem set of rules no-anonymous-default-export and forcing the name of the import to match the name of the export.

auth-service/index.ts

const AuthService = {};

export default AuthService;

auth-service/violation.ts

import Service from '.';

auth-service/good.ts

import AuthService from '.';

@JoshuaKGoldberg JoshuaKGoldberg added Status: Accepting PRs and removed Status: In Discussion Please continue discussing the proposed change before sending a pull request. labels Mar 2, 2019
@JoshuaKGoldberg
Copy link
Author

☠️ It's time! ☠️

Per #876, this repository is no longer accepting feature pull requests. TSLint is being deprecated and we recommend you switch to https://typescript-eslint.io.

Thanks for open sourcing with us, everyone!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants