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

Cannot read property 'forEach' of undefined when iterating html files #2210

Closed
skoblenick opened this issue Aug 26, 2021 · 8 comments
Closed

Comments

@skoblenick
Copy link

Linting all files with a global rule results in:

Linting "demo"...
Cannot read property 'forEach' of undefined
Occurred while linting ./apps/demo/src/app/app.component.html:1

I have a sample repo that reproduces the issue here: https://github.com/skoblenick/eslint-plugin-import-issue

I realize I could technically move the plugin into an override that is specific to the .ts files however it shouldn't be necessary when the code could just be more defensive. I could imagine a use case where I may have preprocessor working on imports within a html or template file (like an hbs) and I want them ordered (hence a lint check).

@ljharb
Copy link
Member

ljharb commented Aug 26, 2021

It definitely should be necessary if you’re linting non-js files; typically those use an entirely separate config for reasons like this.

@skoblenick
Copy link
Author

I have updated my repo and disabled the nx command caching.

The error seems to related to the rule import/first: error. If I disable this in the .eslintrc.yml the linter runs as expected (with lint errors due to the generated files).

@ljharb
Copy link
Member

ljharb commented Aug 27, 2021

I'm not familiar with nx, and i don't see any linting-specific script in that repo's package.json.

npx eslint . fails, but since --ext has to be passed on the command line, i'm not sure how it's lining a .html file with that invocation. I do see the .html overrides section, but you'd need to disable a ton of rules in there since the primary purpose, default, and majority of rules are only for JS or similar code, not HTML.

What's happening is that you're getting very lucky - most rules don't use the Program visitor, and HTML files probably have AST node names that are completely distinct from JS ones. The import plugin has 15 rules using the Program visitor; you might just not have those enabled yet (the airbnb config enables most of them; i suggest using it).

The fix would be trivial - if (!body) { return; } - but i'd want a regression test for it. If you can make a PR that has a failing test case, I'd be happy to tack on the fix and get it in.

ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Nov 23, 2021
ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Nov 23, 2021
ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Nov 23, 2021
ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Nov 23, 2021
ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Nov 23, 2021
ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Nov 23, 2021
ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Nov 23, 2021
ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Nov 23, 2021
ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Nov 23, 2021
@ljharb ljharb closed this as completed in 3875392 Nov 23, 2021
@skoblenick
Copy link
Author

skoblenick commented Jan 13, 2022

So I see a fix in a third party repo but I don't see this merged into main

@skoblenick
Copy link
Author

@ljharb did you submit a PR back to the origin from your fork?

@Dima-Astreyko
Copy link

@ljharb hi, any updates on this bug? Do you want to move this commit to the original repo?

@ljharb
Copy link
Member

ljharb commented Dec 2, 2022

@skoblenick that's inaccurate; the commit is indeed in main, and it's not possible for the commit to have closed this issue unless it landed in main in this repo, so I'm not sure what your confusion is.

@ljharb
Copy link
Member

ljharb commented Dec 2, 2022

@Dima-Astreyko the fix is included in v2.25.4+.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants