Add prefer-import-module-contents custom rule
#37
Conversation
fix config; ouch make exception for propTypes allow hoc and context add json
marcaaron
force-pushed
the
marcaaron-addImportRule
branch
from
afc71e8 to
e1edb2c
Compare
|
Hey @jasperhuangg adding you here since you got PullerBeared on Expensify/App#6279 We need to merge this PR and publish the changes to NPM before we can review that one. There are a LOT of changes there so let me know if you think we should add some more reviewers. |
marcaaron
requested review from
aldo-expensify
and removed request for
jasperhuangg
|
@jasperhuangg is busy so @aldo-expensify you got the lucky spin of Puller Bear 😄 |
|
Just read through this GH issue to be up to date of the decisions made. I'll finish reviewing this tomorrow morning! There are some conflicts you may need to resolve here: Expensify/App#6279 |
Thanks I expect more soon :) will do them all at once as soon as we are done with this. |
| /** | ||
| * @param {String} source | ||
| * @returns {Boolean} | ||
| */ | ||
| function isHigherOrderComponent(source) { | ||
| return /with/.test(source.toLowerCase()); | ||
| } | ||
|
|
||
| /** | ||
| * @param {String} source | ||
| * @returns {Boolean} | ||
| */ | ||
| function isContextComponent(source) { | ||
| return /context|provider/.test(source.toLowerCase()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Can you explain why we're ignoring these types of components?
There was a problem hiding this comment.
I guess we can re-visit whether we want to do this or not later. But basically there are some really common patterns where an HOC is a default export and it's propTypes or something else is a named export which makes it harder to follow the advice to use import * as syntax.
I couldn't work out how to improve this so opted to just make exceptions for things that are basically "internal" libraries like HOCs or Providers. My thinking is that it is OK to treat these like 3rd party libs or Onyx - but mostly I am lazy and didn't want to update too many things at once.
There was a problem hiding this comment.
This seems to be working well. I tried it by changing my package json with:
"eslint-config-expensify": "git+https://github.com/Expensify/eslint-config-expensify.git#e1edb2cfe7fd91b9b131187cb19481ba9d29705b",
Then tested exceptions commenting them out to see the red underline appear. If you modify the linter configuration files, you can force the linter to reload by doing CMD + SHIFT + P and selecting the "Developer: Reload Window" command.
Should be tested in connection with Expensify/App#6279
Adds a rule to enforce the
import * as Something from './somewhere'syntax for most things. There are a few exceptions:/node_modulesas we can't control 3rd party APIspropTypesbecause they are sometimes bundled with components and it is convenient to break outpropTypesanddefaultPropsprovider|context) for the same reason.jsonfile imports because it is convenient to break off a single property from something in a JSON filePart 1 of 2
Expensify/App#6072