Performance increase for no-absolute-path - #843
Conversation
importTypes helper performs resolve() - which does not look needed for the no-absolute-path rule. Additionally, the absolute condition only seems like it was used for no-absolute-path. Finally, all the tests continue to pass.
The old name seems wrong. Probably copied from no-extraneuous-dependencies
|
@benmosher, what do you think of this change? If it's not useful/needed, then I will close the PR. Thanks! |
benmosher
left a comment
There was a problem hiding this comment.
looks great in practice, but see my comments for a lower-impact refactor 😁 thanks for digging into this!
| return () => value | ||
| } | ||
|
|
||
| function isAbsolute(name) { |
There was a problem hiding this comment.
instead of removing this, could you export it and then import it in no-absolute-path?
| } | ||
|
|
||
| const typeTest = cond([ | ||
| [isAbsolute, constant('absolute')], |
There was a problem hiding this comment.
also keep this here, needed for import/order AFAIK
| } | ||
| } | ||
|
|
||
| function isAbsolute(name) { |
There was a problem hiding this comment.
so this is replaced with import { isAbsolute } from '../core/importType'
| describe('importType(name)', function () { | ||
| const context = testContext() | ||
|
|
||
| it("should return 'absolute' for paths starting with a /", function() { |
There was a problem hiding this comment.
and then put these tests back 😁
|
@benmosher I missed your comments. Do you still want to make the suggested refactoring or was it done in another PR/commit? Sorry - been a bit busy due to holidays and other priorities - and somehow I completely missed your messages. |
* origin/master: (66 commits) [Fix] unescape unnecessarily escaped regex slashes [Dev Deps] dev dep ranges should match peer dep ranges docs(readme): add space (import-js#888) bump to v2.7.0 changelog note for import-js#843 upgraded no-absolute-path to use `moduleVisitor` pattern to support all module systems PR note fixes bump to v2.6.1 to bump dep on node resolver to latest 😳 Update no-extraneous-dependencies.md (import-js#878) Fix flow interface imports giving false-negative with `named` rule bump to 2.6.0, node/0.3.1, webpack/0.8.3, memo-parser/0.2.0 chore(eslint): upgrade to eslint@4 memo-parser: require eslint >= 3.5.0 (need file path always) build on node v4, again (import-js#855) bump to v2.5.0 bump `debug` version everywhere resolvers/webpack: v0.8.2 eslint-module-utils: v2.1.1 (bumping to re-publish to npm) [Tests] comment out failing (and probably invalid) test Only apps should have lockfiles. ...
See my reply here for a detailed analysis on why I made this change: #803 (comment)
I remove the call to
resolve()inno-absolute-pathrule. That should make this rule faster when it is used as a standalone rule or with other rules that are not dependent on having to resolve file paths.As I currently see no reason to have to call
resolve()for this rule at all. However, I am new to the code, so I may be wrong. :)