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

fix: eslint module path resolver for yarn workspaces #42

Closed
wants to merge 2 commits into from
Closed

fix: eslint module path resolver for yarn workspaces #42

wants to merge 2 commits into from

Conversation

igl
Copy link

@igl igl commented Apr 25, 2018

Instead of looking for the closest node_modules, it will resolve the path like require.resolve() starting from config.rootDir.

Fixes #41
All tests pass
Rejoice

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Please add some tests.

package.json Outdated
"pify": "3.0.0",
"resolve-from": "4.0.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please do not se this module; you should be able to achieve this with resolve

Copy link
Author

Choose a reason for hiding this comment

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

Oh and that is your module, I see.

Starting from node 8.9 adding lookup paths is available in core. Does it fall back to the built-in fn?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I maintain it; I didn't create it.

No, it doesn't fall back to the built in function, but that's an implementation detail.

Copy link
Author

Choose a reason for hiding this comment

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

tomato potato. Jest actually comes with it's own jest-resolve package which seems to be a fork of substack/node-resolve.

They did not write docs about it at all, so i would refrain from using that anyhow.

@igl
Copy link
Author

igl commented Apr 26, 2018

It's using resolve now.
I added a basic unit test.

Should i do the work and add a proper integration test to check my mono-repo scenario?
I will have to allow a fake node_modules folder to be in the fixtures folder.
Not sure how to go about that yet.
Maybe programmatically create it when the test runs?

@ljharb
Copy link
Collaborator

ljharb commented Apr 26, 2018

I think it’s fine to add a fake node_modules and package.json into the repo inside the tests dir

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good overall - but we do indeed want a test of your use case :-)

const requiredESLint = require('eslint');

it('requires eslint', () => {
// eslint-disable-next-line global-require
Copy link
Collaborator

Choose a reason for hiding this comment

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

This override shouldn’t be needed?

});

it('finds eslint with custom rootDir', () => {
// eslint-disable-next-line global-require
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nor this one?

@tomasz-sodzawiczny
Copy link

@igl Why is this PR stuck? Can I help somehow? I'm also waiting for this to work with yarn workspaces. :)

@igl igl closed this Nov 20, 2018
@igl igl deleted the fix_eslint_require_resolver branch November 20, 2018 20:34
@ljharb
Copy link
Collaborator

ljharb commented Nov 20, 2018

@igl are you no longer interested in pursuing this?

@sibelius
Copy link

can't eslint solve this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Yarn workspaces
4 participants