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

Open
2 of 4 tasks
amareshsm opened this issue Sep 9, 2024 · 5 comments · May be fixed by #626
Open
2 of 4 tasks

Bug: eslint-scope requires assert #625

amareshsm opened this issue Sep 9, 2024 · 5 comments · May be fixed by #626
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
@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
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 linked a pull request Sep 12, 2024 that will close this issue
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
Status: Implementing
Development

Successfully merging a pull request may close this issue.

3 participants