Skip to content

Conversation

olafurpg
Copy link
Contributor

Previously, scip-typescript only emitted occurrences for identifiers. Now, we additionally emit occurrences for string literals so that features like "Go to definition" work with import statements and other locations where string literals reference actual symbols.

Test plan

See the updated snapshot tests. I additionally manually verified that it works as expected on Sourcegraph https://sourcegraph.com/github.com/sourcegraph/scip-typescript@324cb332db75b3082fdbccc45a1a8e166d546b28/-/blob/src/FileIndexer.ts?L21:28&popover=pinned

CleanShot 2022-10-18 at 11 39 31@2x

Previously, scip-typescript only emitted occurrences for identifiers.
Now, we additionally emit occurrences for string literals so that
features like "Go to definition" work with `import` statements and other
locations where string literals reference actual symbols.
path
.basename(this.sourceFile.fileName)
.replace(/.tsx?$/, '')
.replace(/.jsx?/, '')
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Is it ever possible that we have files with the same base name but with different extensions? IIUC this happens with translation/conversion, but we will not index the generated files.
  2. I think you need to add .d.ts, .cjs and .mjs extensions too.
  3. Since the . is inside the regex, it needs escaping, right? Or is this some reason why it doesn't need to be escaped here?

I'm wondering, instead of dropping the extension here, can we get the filename and include the extension when emitting the occurrence for the import instead? Any reason to prefer dropping the extension instead of doing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case should not happen since it's a fallback when moduleName is undefined, and I'm guessing it's always defined for ts.SourceFile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My preference for dropping the extension was to best-effort mirror the output of moduleName, even if it doesn't handle all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out moduleName was in fact undefined almost everywhere! I updated the logic to keep the file extension so that we don't treat d.ts/mjs/cjs extensions differently

@olafurpg olafurpg merged commit d9e969c into main Oct 18, 2022
@olafurpg olafurpg deleted the olafurpg/string-literals branch October 18, 2022 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants