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

Performance increase for no-absolute-path #843

Merged
merged 2 commits into from
Jul 6, 2017

Conversation

jseminck
Copy link

See my reply here for a detailed analysis on why I made this change: #803 (comment)

I remove the call to resolve() in no-absolute-path rule. 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. :)

Joachim Seminck added 2 commits May 24, 2017 18:37
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
@coveralls
Copy link

coveralls commented May 24, 2017

Coverage Status

Coverage decreased (-0.01%) to 95.048% when pulling 5aa2fe0 on jseminck:no-absolute-path-perf into b79e083 on benmosher:master.

@jseminck
Copy link
Author

@benmosher, what do you think of this change? If it's not useful/needed, then I will close the PR. Thanks!

Copy link
Member

@benmosher benmosher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great in practice, but see my comments for a lower-impact refactor 😁 thanks for digging into this!

@@ -8,10 +8,6 @@ function constant(value) {
return () => value
}

function isAbsolute(name) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of removing this, could you export it and then import it in no-absolute-path?

@@ -50,7 +46,6 @@ function isRelativeToSibling(name) {
}

const typeTest = cond([
[isAbsolute, constant('absolute')],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also keep this here, needed for import/order AFAIK

context.report(node, 'Do not import modules using an absolute path')
}
}

function isAbsolute(name) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this is replaced with import { isAbsolute } from '../core/importType'

@@ -8,12 +8,6 @@ import { testContext } from '../utils'
describe('importType(name)', function () {
const context = testContext()

it("should return 'absolute' for paths starting with a /", function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and then put these tests back 😁

@jseminck
Copy link
Author

jseminck commented Jul 7, 2017

@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.

SpainTrain pushed a commit to SpainTrain/eslint-plugin-import that referenced this pull request Aug 2, 2017
* 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.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants