Adds no-namespaced-import rule
#262
Conversation
cartogram
force-pushed
the
no-namespaced-import
branch
2 times, most recently
from
cc17a25
to
43a1759
Compare
|
Approach LGTM. |
cartogram
force-pushed
the
no-namespaced-import
branch
from
169f86a
to
7e88f34
Compare
cartogram
changed the title
no-namespaced-import ruleno-namespaced-import rule
cartogram
force-pushed
the
no-namespaced-import
branch
3 times, most recently
from
da0bd38
to
fe9cf0d
Compare
docs/rules/no-namespace-imports.md
Outdated
| @@ -0,0 +1,46 @@ | |||
| # Prevent namespace import declarations. (no-namespace-imports) | |||
|
|
|||
| Namespace imports grab all exported variables from a module as nested properties under a single variable name. This approach of importing everything, rather than only what is required from the module, leads to a polluted local scope from the unused components that were brought in under the module namespace. | |||
There was a problem hiding this comment.
It does not polite the local scope, that's half the benefit of namespace imports. If we're going to sell this on anything, it should be that this style of import breaks tree shaking.
docs/rules/no-namespace-imports.md
Outdated
|
|
||
| ```ts | ||
|
|
||
| import React from 'react'; |
There was a problem hiding this comment.
I'd rather show an example showing named imports > namespace import. Yes, this rule does help us enforce the right style for React, but that's something that could conceivably be addressed in types instead.
docs/rules/no-namespace-imports.md
Outdated
| "shopify/no-namespace-imports": [ | ||
| "error", | ||
| { | ||
| "allow": ["react", "some-custom-module"] |
There was a problem hiding this comment.
Probably best not to show it for React, we'd never want them to do that, would we?
lib/rules/no-namespace-imports.js
Outdated
| node, | ||
| messageId: 'namespaceImport', | ||
| data: {name}, | ||
| fix: (fixer) => { |
There was a problem hiding this comment.
I don't think it's safe to provide a fixer, we have no idea what members they reference, and turning it into a default import is not going to be correct in many cases.
lib/rules/no-namespace-imports.js
Outdated
|
|
||
| function ignoreModule(node, allowed) { | ||
| return allowed.some((stringOrRegex) => { | ||
| const regex = new RegExp(stringOrRegex); |
There was a problem hiding this comment.
Better to construct this regex array upfront.
|
superseded by #305 |
|
After trying the import/no-namespace rule, I do think we want a custom rule after all. I'll continue with this work and get it ready for another review. |
cartogram
force-pushed
the
no-namespaced-import
branch
from
fe9cf0d
to
eaf56c4
Compare
cartogram
force-pushed
the
no-namespaced-import
branch
from
9e43193
to
ee08c54
Compare
cartogram
force-pushed
the
no-namespaced-import
branch
2 times, most recently
from
210039f
to
80063ce
Compare
This PR adds a new rule to prevent namespace imports.
ToDo:
Reviewers:
I'd like reviewers to focus on the following:
import * as foo from 'Foo'might actually want to beimport {foo, bar, baz} from 'Foo'. The fixer could also lead to breaking code.allowoption. Is this enough configuration or should we an an array option to match modules to report (the opposite of allow).