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: Properly export module resolver #34

Merged
merged 5 commits into from
Apr 30, 2021
Merged

Conversation

Richienb
Copy link
Contributor

@Richienb Richienb commented Apr 28, 2021

relative-module-resolver.js does not provide a named export:

module.exports = {
/**
* Resolves a Node module relative to another module
* @param {string} moduleName The name of a Node module, or a path to a Node module.
* @param {string} relativeToPath An absolute path indicating the module that `moduleName` should be resolved relative to. This must be
* a file rather than a directory, but the file need not actually exist.
* @returns {string} The absolute path that would result from calling `require.resolve(moduleName)` in a file located at `relativeToPath`
*/
resolve(moduleName, relativeToPath) {
try {
return createRequire(relativeToPath).resolve(moduleName);
} catch (error) {
// This `if` block is for older Node.js than 12.0.0. We can remove this block in the future.
if (
typeof error === "object" &&
error !== null &&
error.code === "MODULE_NOT_FOUND" &&
!error.requireStack &&
error.message.includes(moduleName)
) {
error.message += `\nRequire stack:\n- ${relativeToPath}`;
}
throw error;
}
}
};

However it is still imported like so:

const { ModuleResolver } = require("./shared/relative-module-resolver");

This PR fixes it by properly reexporting the fle. It is not a breaking change because even though the variable is renamed because the original name will still resolve with undefined.

@eslint-github-bot
Copy link

Hi @Richienb!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag must be one of the following:

    The Tag is one of the following:

    • Fix - for a bug fix.
    • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
    • New - implements a new feature.
    • Breaking - for a backwards-incompatible enhancement or feature.
    • Docs - changes to documentation only.
    • Build - changes to build process only.
    • Upgrade - for a dependency upgrade.
    • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

    You can use the labels of the issue you are working on to determine the best tag.

  • There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

2 similar comments
@eslint-github-bot
Copy link

Hi @Richienb!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag must be one of the following:

    The Tag is one of the following:

    • Fix - for a bug fix.
    • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
    • New - implements a new feature.
    • Breaking - for a backwards-incompatible enhancement or feature.
    • Docs - changes to documentation only.
    • Build - changes to build process only.
    • Upgrade - for a dependency upgrade.
    • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

    You can use the labels of the issue you are working on to determine the best tag.

  • There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

@eslint-github-bot
Copy link

Hi @Richienb!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag must be one of the following:

    The Tag is one of the following:

    • Fix - for a bug fix.
    • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
    • New - implements a new feature.
    • Breaking - for a backwards-incompatible enhancement or feature.
    • Docs - changes to documentation only.
    • Build - changes to build process only.
    • Upgrade - for a dependency upgrade.
    • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

    You can use the labels of the issue you are working on to determine the best tag.

  • There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

@Richienb Richienb changed the title Fix reexport Fix: Properly export module resolver Apr 28, 2021
@eslint-github-bot
Copy link

Hi @Richienb!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag must be one of the following:

    The Tag is one of the following:

    • Fix - for a bug fix.
    • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
    • New - implements a new feature.
    • Breaking - for a backwards-incompatible enhancement or feature.
    • Docs - changes to documentation only.
    • Build - changes to build process only.
    • Upgrade - for a dependency upgrade.
    • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

    You can use the labels of the issue you are working on to determine the best tag.

  • There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

seems it's lacking of tests. can you add some tests? thx!

@aladdin-add aladdin-add added bug Something isn't working and removed triage labels Apr 28, 2021
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

lib/index.js Outdated Show resolved Hide resolved
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Agree with @mdjermanovic -- we should stick with the name ModuleResolver

@nzakas
Copy link
Member

nzakas commented Apr 28, 2021

Note: We probably didn't catch this earlier because in ESLint we overwrite ModuleResolver to use the one in the ESLint repo.

Signed-off-by: Richie Bendall <richiebendall@gmail.com>
@mdjermanovic
Copy link
Member

Note: We probably didn't catch this earlier because in ESLint we overwrite ModuleResolver to use the one in the ESLint repo.

We're using this one, but with require("@eslint/eslintrc/lib/shared/relative-module-resolver"). When we release this fix, we could update that in the eslint/eslint repo.

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@nzakas nzakas merged commit aa38ef4 into eslint:main Apr 30, 2021
btmills added a commit to eslint/eslint that referenced this pull request May 8, 2021
This incorporates eslint/eslintrc#34, which
allows fixing `CLIEngine`'s `ModuleResolver` import.
btmills added a commit to eslint/eslint that referenced this pull request May 8, 2021
This incorporates eslint/eslintrc#34, which
allows fixing `CLIEngine`'s `ModuleResolver` import.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants