Skip to content

Conversation

@Richienb
Copy link
Contributor

CommonJS and ESM support is mutually exclusive for the code so I left upgrade notes instead.

Automatically testing for the issue mentioned in the original report is unfeasible so it will need to be tested for manually.

Fixes: #543

Signed-off-by: Richie Bendall <[email protected]>
};

// TODO: Use `resolveModule(normalizePackageName(name), import.meta.url);` when moving to ESM then to `import.meta.resolve(normalizePackageName(name))` when supported
const resolveLocalConfig = name => resolveModule(normalizePackageName(name, 'eslint-config'), require.main.filename);
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be more correct to use path.resolve('..', __dirname) instead of require.main.filename?

I'm not sure how resolveModule works, but doesn't resolveModule(normalizePackageName(name), import.meta.url); also need to search from the parent directory of import.meta.url? Regarding the code comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't resolveModule(normalizePackageName(name), import.meta.url); also need to search from the parent directory of import.meta.url?

resolveModule just wraps require("module").createRequire(path).resolve(name) which is the same code that is recommended to be used in a related StackOverflow question.

Wouldn't it be more correct to use path.resolve('..', __dirname) instead of require.main.filename?

Better still, we could just use name => require.resolve(normalizePackageName(name)).

@xojs xojs deleted a comment from codecov-commenter May 11, 2021
@sindresorhus sindresorhus merged commit d2c5750 into xojs:main May 11, 2021
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.

Error: Cannot find module 'eslint-config-xo'

2 participants