eslint / js

Monorepo for the JS language tools.
BSD 2-Clause "Simplified" License
2.29k stars 196 forks source link

Bug: `eslint-scope` requires `assert` #625

Closed amareshsm closed 2 months ago

amareshsm commented 2 months ago

Which packages are affected?

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.

https://github.com/eslint/js/blob/79c6e93ba93b46186ebfac111c94f44ab3b4ee27/packages/eslint-scope/lib/scope-manager.js#L187

Also assert package was intentionally ignored in the config. https://github.com/eslint/js/blob/79c6e93ba93b46186ebfac111c94f44ab3b4ee27/packages/eslint-scope/rollup.config.js#L3

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

Additional comments

No response

aladdin-add commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Sure. I will look into this.

amareshsm commented 2 months ago

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 commented 2 months ago

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

Yes.