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

Bug: eslint-scope requires assert #625

Closed
2 of 4 tasks
amareshsm opened this issue Sep 9, 2024 · 5 comments · Fixed by #633
Closed
2 of 4 tasks

Bug: eslint-scope requires assert #625

amareshsm opened this issue Sep 9, 2024 · 5 comments · Fixed by #633
Assignees
Labels
accepted bug repro:needed This issue should include a reproducible example

Comments

@amareshsm
Copy link
Member

amareshsm commented Sep 9, 2024

Which packages are affected?

  • espree
  • eslint-scope
  • eslint-visitor-keys

Environment

Node version: v20.16.0
npm version: 10.8.1
ESLint version: 8.57.0
Operating System: macOS

What did you do?

In Code Explorer, I aimed to clean up the dependencies by removing unused development and runtime dependencies. I identified the dependencies that were not being used in the repository and then removed them.

What did you expect to happen?

Code Explorer should work as expected since I removed only the unused dependencies.

What actually happened?

code explorer scope view was breaking got the below error:
Image

When I reviewed the scope-manager logic in response to the error, I found that it utilizes assert.

assert(this.__currentScope === null);

Also assert package was intentionally ignored in the config.

external: ["assert", "estraverse", "esrecurse"],

This means the assert package was intentionally marked as external and is expected to be present in the environment where the package is used.

Do we have a specific reason for marking assert as external? There might be cases where assert is not available in the environment where the package (scope-manager) is being used.

Link to Minimal Reproducible Example

https://deploy-preview-33--eslint-code-explorer.netlify.app

Participation

  • I am willing to submit a pull request for this issue.

Additional comments

No response

@amareshsm amareshsm added bug repro:needed This issue should include a reproducible example labels Sep 9, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Sep 9, 2024
@amareshsm amareshsm changed the title Bug: eslint-scope scope requires assert Bug: eslint-scope requires assert Sep 9, 2024
@aladdin-add
Copy link
Member

aladdin-add commented Sep 10, 2024

Do we have a specific reason for marking assert as external? There might be cases where assert is not available in the environment where the package (scope-manager) is being used.

it's to avoid the rollup warnings: https://rollupjs.org/troubleshooting/#warning-treating-module-as-external-dependency. I'd suggest not using assert, but just throw an error:

if(this.__currentScope !== null) {throw new Error("an error happened.")}

@nzakas nzakas moved this from Needs Triage to Ready to Implement in Triage Sep 10, 2024
@nzakas
Copy link
Member

nzakas commented Sep 10, 2024

This is left over from when we forked escope. I don't think there's any reason to reference the assert package.

I think the easiest thing is just to create and use our own assert function:

function assert(condition, message = "Assertion failed.") {
    if (!condition) {
        throw new Error(message);
    }
}

And then replace the references to assert package with this function.

@amareshsm do you want to take a look at this?

@amareshsm
Copy link
Member Author

Sure. I will look into this.

@amareshsm
Copy link
Member Author

This is left over from when we forked escope. I don't think there's any reason to reference the assert package.

I think the easiest thing is just to create and use our own assert function:

function assert(condition, message = "Assertion failed.") {
if (!condition) {
throw new Error(message);
}
}
And then replace the references to assert package with this function.

@amareshsm do you want to take a look at this?

The test files also contain references. Should we leave them as is, since they won’t be included in the bundle?

@nzakas
Copy link
Member

nzakas commented Sep 12, 2024

The test files also contain references. Should we leave them as is, since they won’t be included in the bundle?

Yes.

@amareshsm amareshsm moved this from Ready to Implement to Implementing in Triage Sep 12, 2024
@github-project-automation github-project-automation bot moved this from Implementing to Complete in Triage Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted bug repro:needed This issue should include a reproducible example
Projects
Archived in project
3 participants