Skip to content

Handle jsx runtime implicit synthetic import in find all references #51319

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

Merged
merged 7 commits into from
Mar 23, 2023

Conversation

weswigham
Copy link
Member

Fixes #51221

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Oct 26, 2022
const range = forEachChildRecursively(
reference.referencingFile,
n => isJsxElement(n) || isJsxSelfClosingElement(n) || isJsxFragment(n) ? n : undefined
) || reference.referencingFile.statements[0] || reference.referencingFile;
Copy link
Member

Choose a reason for hiding this comment

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

I thought a bit more about the statements[0] case. It would be very weird if the first statement in your file is an unrelated import...

Maybe we need to find the first statement that's not an import or an export with a module specifier?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure it matters - import "./thing" being the ref is probably just as odd as var x = 1. Point is really that the file itself is the reference, we just don't want to say "the whole file is the reference" because it'd be a really really big and useless usage highlight span.

@weswigham weswigham force-pushed the fix-implicit-jsx-refernce-crash branch from 07a6a54 to 86dab1d Compare January 10, 2023 21:40
@typescript-bot typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Mar 22, 2023
Copy link
Member

@sheetalkamat sheetalkamat left a comment

Choose a reason for hiding this comment

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

Because tslib came up as synthesized node, can we add test for that.

@@ -0,0 +1,38 @@
// === findAllReferences ===
// === /tests/cases/fourslash/project/src/foo.ts ===
// [|<|import * as x from /*FIND ALL REFS*/"[|tslib|]";|]|>
Copy link
Member

Choose a reason for hiding this comment

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

Since we are already highlighting import do we really want first statement as well ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep - there's two references in this file, after all - the implicit one and the explicit one.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe there should be just one if the file already has an explicit import.

Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

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

The behavior could be better, but I guess I just want the crash gone already.

@weswigham weswigham merged commit 916f9b7 into microsoft:main Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Start < 0 in BuilderIO/qwik
5 participants