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

[compiler/language-service::getScriptVersion] Shouldn't this statement use optional chaining and not non-null assertion operator? #1639

Closed
cp79shark opened this issue May 13, 2020 · 4 comments · Fixed by #1641
Labels
🐛 Bug Confirmed Bug is confirmed

Comments

@cp79shark
Copy link

const version = memoryCache.files.get(normalizedFileName)!.version

If the file is not in the MemoryCache this will throw. I'm not sure if this is the intended behavior, but it is causing issues for me.

// We need to return `undefined` and not a string here because TypeScript will use
// `getScriptVersion` and compare against their own version - which can be `undefined`.
// If we don't return `undefined` it results in `undefined === "undefined"` and run
// `createProgram` again (which is very slow). Using a `string` assertion here to avoid
// TypeScript errors from the function signature (expects `(x: string) => string`).
return version === undefined ? ((undefined as any) as string) : String(version)

The return statement, and the lengthy comment above it, already expects as much is a possibility and will return undefined.

Changing to const version = memoryCache.files.get(normalizedFileName)?.version; will allow a cache miss and return undefined.

@ahnpnl
Copy link
Collaborator

ahnpnl commented May 14, 2020

Do you have a case when it causes issue ? getScriptVersion is always executed after getScriptFileName (as far as I have observed) so that’s why the ! operator is there.

I’m curious to see which cases it fails. But ya for code perspective, ? should be used instead

@cp79shark
Copy link
Author

Heh, I woke up this morning trying to think how I could provide you a minimal repro (source repo is huge and private). The error was only reproducible with a clean repo (delete entire root folder, clone, jest). normalizedFileName was set, just not present in cache. I didn't dig into it too much on the why side since the "fix" was obvious and the sirens of other fires were calling my name.

If there's any info I can provide let me know, but with the PR I'm good. Will just patch local JS until update published. Thanks.

@johnjayhopley
Copy link

Has a fix been published for this now?

@ahnpnl
Copy link
Collaborator

ahnpnl commented May 15, 2020

will be soon, included in 26.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug Confirmed Bug is confirmed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants