Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issues in fourslash tests related to invalid JSON syntax and incorrect completions API usage. The changes ensure that package.json files in test fixtures are valid JSON (removing trailing commas) and that completion preferences are passed correctly to the verify.completions API.
- Removes trailing commas from package.json files to ensure valid JSON syntax
- Fixes incorrect preferences passing in verify.completions calls where preferences were mistakenly placed as a separate argument instead of within the options object
- Adds runtime validation to catch future instances of incorrect preferences passing
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/cases/fourslash/server/importSuggestionsCache_invalidPackageJson.ts | Removes trailing comma from jsconfig.json |
| tests/cases/fourslash/organizeImportsReactJsxDev.ts | Removes trailing comma from react package.json |
| tests/cases/fourslash/organizeImportsReactJsx.ts | Removes trailing comma from react package.json |
| tests/cases/fourslash/completionsWithDeprecatedTag9.ts | Fixes preferences passing and adds missing completion entries |
| tests/cases/fourslash/completionsThisProperties_globalType.ts | Fixes preferences passing syntax |
| tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports48.ts | Removes trailing comma from react package.json |
| tests/cases/fourslash/codeFixMissingTypeAnnotationOnExports47.ts | Removes trailing comma from react package.json |
| tests/cases/compiler/jsxClassAttributeResolution.tsx | Removes trailing comma from @types/react package.json |
| tests/baselines/reference/jsxClassAttributeResolution.js | Updates baseline to match fixed package.json |
| tests/baselines/reference/jsxClassAttributeResolution.errors.txt | Updates baseline to match fixed package.json |
| src/harness/fourslashInterfaceImpl.ts | Adds validation to detect incorrect preferences passing pattern |
| for (const options of optionsArray) { | ||
| if (options.preferences && Object.keys(options).length === 1 && optionsArray.length > 1 && optionsArray.indexOf(options) > 0) { |
There was a problem hiding this comment.
Using optionsArray.indexOf(options) within a loop has O(n²) complexity. Store the index from the for...of loop instead. Change for (const options of optionsArray) to for (let i = 0; i < optionsArray.length; i++) and use i > 0 in the condition.
| for (const options of optionsArray) { | |
| if (options.preferences && Object.keys(options).length === 1 && optionsArray.length > 1 && optionsArray.indexOf(options) > 0) { | |
| for (let i = 0; i < optionsArray.length; i++) { | |
| const options = optionsArray[i]; | |
| if (options.preferences && Object.keys(options).length === 1 && optionsArray.length > 1 && i > 0) { |
There was a problem hiding this comment.
Not a completely wild review comment but not a concern here
| @@ -3,7 +3,7 @@ | |||
| // @Filename: /home/src/workspaces/project/jsconfig.json | |||
| ////{ | |||
| //// "compilerOptions": { | |||
| //// "module": "commonjs", | |||
| //// "module": "commonjs" | |||
There was a problem hiding this comment.
This is a jsconfig so probably okay, but NBD
There was a problem hiding this comment.
Oh yeah, I just modified readJson and went to town
There was a problem hiding this comment.
Does that mean jsconfigs are not using JSONC? Fun
|
@typescript-bot cherry-pick to tsgo-port |
|
Hey, @andrewbranch! I've created #62642 for you. |
|
Lol, one of them is named "importSuggestionsCache_invalidPackageJson" so I better revert that one too |
|
Oh, it’s the same one I changed the jsconfig on |
|
These timeouts are getting annoying... |
Fixes incorrect completions preferences passing and trailing commas in package.jsons