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

Custom resolver path #314

Merged
merged 10 commits into from
Jun 1, 2016
Merged

Custom resolver path #314

merged 10 commits into from
Jun 1, 2016

Conversation

le0nik
Copy link
Contributor

@le0nik le0nik commented May 7, 2016

With this PR users will be able to require resolvers:

  1. With absolute paths (/Volumes/..., user can include them as computed property in .eslintrc.js config)
  2. With paths, relative to closest package.json (./eslint-plugins/webpack-resolver)
  3. With custom npm modules names (@myorg/webpack-resolver)
  4. With conventional npm modules names (webpack -> eslint-import-resolver-webpack)

@lukeapage
Copy link
Contributor

Hi,

this is similiar to my #312 and does alot more. But it doesn't allow a project relative path which I needed (absolute paths are useless for shared projects)

@lukeapage
Copy link
Contributor

ah sorry I understand - the user can calculate the absolute path in the config and pass that. I guess its because we don't know the path of the .eslintrc file ? I just went with resolving to cwd

@le0nik
Copy link
Contributor Author

le0nik commented May 7, 2016

@lukeapage What do you mean? See 2). You can pass relative path, e.g. ./my-resolver-plugin. It will calculate it from your root project directory.

@le0nik
Copy link
Contributor Author

le0nik commented May 7, 2016

@lukeapage I saw your PR. I don't think it works at all, to be honest :/.

On this line: https://github.com/benmosher/eslint-plugin-import/pull/312/files#diff-2bd490f6c0bd766af6e36e61189196c8R148 you check if the path is absolute(which would be for example /Volumes/my-project/...). If true, this path would be appended to eslint-import-resolver- and become eslint-import-resolver-/Volumes/my-project/..., which isn't really what you want.

I decided to go another way with this PR and add some more options(my requirement is to also allow local resolvers and private resolver packages).

@lukeapage
Copy link
Contributor

Ah i see how (2) works sorry, i replied first thing in the morning, must have still been asleep.

Yes your pr is superior, had just missed (2).

@benmosher
Copy link
Member

Sweet! I pulled requireResolver out to a function with exactly this in mind. 😁 Hadn't gotten around to it.

I will look more closely when I get a chance to understand why tests are failing,

@le0nik
Copy link
Contributor Author

le0nik commented May 7, 2016

@benmosher The problem with failing tests seems to be within this function, that doesn't return context.report, where as resolve expects context.report to be a function.

@benmosher
Copy link
Member

That must mean some test is failing to load a resolver and it's trying to report it in the catch handler.

@le0nik
Copy link
Contributor Author

le0nik commented May 7, 2016

@benmosher It was my fault. It's fixed now, but one test still doesn't pass on Node 0.12. I can't reproduce it on my machine with Node 0.12. If you have an idea on how to fix it, i'll gladly do it.

@benmosher
Copy link
Member

Agh, no worries, it's not you, it's Travis. I need to bump up the timeouts or something.

I will double-check before I merge, but it's probably good to go. 😎

@le0nik
Copy link
Contributor Author

le0nik commented May 7, 2016

Ok, thanks! After you check this, tell me if you need me to squash it and I'll do it.

@le0nik
Copy link
Contributor Author

le0nik commented May 16, 2016

Is there anything I can do to get this PR merged?

@lukeapage
Copy link
Contributor

+1 am waiting for a release with this in before I can use this plugin.

@benmosher
Copy link
Member

Sorry, got this confused with #320 and had parked it mentally. I think it's good, would be better if it had tests but the code looks good.

Can you

  • rebase against master
  • update the change log (probably to the Added section)
  • add a note to the README resolvers section that denotes the ways that resolvers can be referenced

Also, if you could

that would be ideal as pkg-dir shares a dep with pkg-up (which is already a dependency), and does roughly the same thing. I am planning to do the same with the find-root reference in the Webpack resolver at some point.

@leonid-bauxy
Copy link

@benmosher done!

@lukeapage
Copy link
Contributor

@benmosher any chance of getting this merged soon now your todo's are done?

@benmosher
Copy link
Member

Yeah, should be good. I need to make another run through the changes but I think it will be good.

@benmosher benmosher merged commit a3d9911 into import-js:master Jun 1, 2016
if (path === undefined) return { found: false }
return { found: true, path }
if (path === undefined) return { found: false, path: null }
return { found: true, path: null }
Copy link
Member

Choose a reason for hiding this comment

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

@le0nik what motivated this change?

Copy link
Contributor Author

@le0nik le0nik Jun 3, 2016

Choose a reason for hiding this comment

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

As i remember, it was due to this failing test after merge of master.

Ah, i see what you mean. My mistake. The test affected just the line above, not this one. I shouldn't have changed this.

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.

4 participants