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] guard against empty parent #2832

Merged
merged 1 commit into from Jul 24, 2023
Merged

[Fix] guard against empty parent #2832

merged 1 commit into from Jul 24, 2023

Conversation

laurens-dg
Copy link
Contributor

Without this fix I get a crash on my current linting set-up.
Not sure what is causing the parent to be empty, might be a bug in the parser, but I ran into this updating everything to the latest versions.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

We’d need a regression test to land this - can you figure out which code is crashing?

@ljharb ljharb marked this pull request as draft July 18, 2023 15:07
@laurens-dg
Copy link
Contributor Author

Absolutely, but I don't know how to go about this, I get this crash for parents with type ExportNamedDeclaration I've got about 20 of those in my project.

The crash looks like this:
ESLint: 8.45.0

TypeError: Cannot read properties of undefined (reading 'indexOf')
Occurred while linting /foo/bar/baz.tsx:18
Rule: "import/newline-after-import"
at checkImport (/foo/bar/node_modules/eslint-plugin-import/lib/rules/newline-after-import.js:155:40)

@ljharb
Copy link
Member

ljharb commented Jul 19, 2023

What's on line 18 of that tsx file?

@laurens-dg
Copy link
Contributor Author

export import Foo = ns.baz.bar.Foo;

We have a bunch of namespaced classes which we rename and re-export in this way.

@ljharb
Copy link
Member

ljharb commented Jul 19, 2023

woof, that is some gnarly TS syntax :-) i know about export = and import = but not export import =. thanks - it shouldn't be too tricky to come up with a test case based on that.

@ljharb
Copy link
Member

ljharb commented Jul 19, 2023

Although in this case, the rule should presumably be enforcing a newline after the export import = line, not ignoring it?

@laurens-dg
Copy link
Contributor Author

laurens-dg commented Jul 24, 2023

To be honest, I don't know.
I also believe this used to work but it might be a recent change to the @typescript-eslint/parser that causes this issue.
A bit further in the code this case is considered and skipped. But we see errors before that check with the indexOf, hence I created this PR.

@ljharb
Copy link
Member

ljharb commented Jul 24, 2023

aha, #1830 indeed attempts to ignore them, treating them as exports rather than imports.

In this case, it sounds like the right bug fix, as long as there's a regression test.

@ljharb ljharb marked this pull request as ready for review July 24, 2023 17:26
@ljharb ljharb merged commit ee00a1c into import-js:main Jul 24, 2023
162 of 163 checks passed
@laurens-dg laurens-dg deleted the patch-1 branch July 26, 2023 07:53
@laurens-dg
Copy link
Contributor Author

Thanks for the help and feedback in getting this merged!

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

Successfully merging this pull request may close these issues.

None yet

2 participants