Enable allowJs when inferred project has root JS files#3432
Enable allowJs when inferred project has root JS files#3432jakebailey wants to merge 5 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes inferred-project behavior for unconfigured JavaScript files by ensuring allowJs is enabled when an inferred project’s roots include JS files, and adjusts the fourslash harness so tests can precisely control which files are “opened” (and thus become inferred roots).
Changes:
- Enable
AllowJs(andAllowNonTsExtensions) for inferred projects whenever any inferred root file is a JS-family extension. - Add a fourslash harness option
@noDefaultOpenplus an exportedOpenFilehelper to avoid auto-opening every test file. - Update/select manual fourslash tests to use
@noDefaultOpenand add a new regression test for completions in an unconfigured.jsfile.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/testutil/harnessutil/harnessutil.go | Adds NoDefaultOpen harness option parsing so tests can opt out of default didOpen behavior. |
| internal/project/projectcollectionbuilder.go | Forces AllowJs when inferred roots include JS while updating inferred project roots. |
| internal/project/project.go | Adds helpers to detect JS roots and clone compiler options with AllowJs enabled; applies in inferred project creation. |
| internal/fourslash/fourslash.go | Implements @noDefaultOpen behavior in NewFourslash, marks openFile as helper, and exposes OpenFile. |
| internal/fourslash/tests/completionsInUnconfiguredJSFile_test.go | New regression test validating completion works in an unconfigured .js file. |
| internal/fourslash/tests/manual/tripleSlashRefPathCompletionExtensionsAllowJSFalse_test.go | Uses @noDefaultOpen and explicit open to prevent unintended inferred roots. |
| internal/fourslash/tests/manual/tripleSlashRefPathCompletionAbsolutePaths_test.go | Uses @noDefaultOpen and explicit open to prevent unintended inferred roots. |
| internal/fourslash/tests/manual/completionForStringLiteralNonrelativeImport2_test.go | Uses @noDefaultOpen and explicit opens for deterministic inferred roots. |
| internal/fourslash/_scripts/manualTests.txt | Adds missing manual test entries so the manual test harness script includes them. |
| // OpenFile opens a file via LSP didOpen, making it a root of the inferred project. | ||
| func (f *FourslashTest) OpenFile(t *testing.T, filename string) { | ||
| t.Helper() | ||
| f.openFile(t, filename) | ||
| } |
There was a problem hiding this comment.
The doc comment for OpenFile is a bit too strong: didOpen doesn’t necessarily make a file a root of the inferred project (if the file ends up belonging to a configured project, it won’t become an inferred root). Consider rewording to describe it as “opens a file via LSP didOpen” and, if relevant, note that it may affect inferred project roots when the file is unconfigured.
Hm, is |
|
It is right at startup, but the options it sends do not include allowJs; I don't think they did in Strada either, and it overrode the compiler options to force these two options on. Could be that we just need to undo that initial step, it's just really wonky given that corsa, strada fourslash, strada fourslash-server all behave differently |
|
LGTM but generate is failing |
| if testData.isStateBaseliningEnabled() { | ||
| // Single baseline, so initialize project state baseline too | ||
| f.stateBaseline = newStateBaseline(fsFromMap.(iovfs.FsWithSys)) | ||
| } else if harnessOptions.NoDefaultOpen { |
There was a problem hiding this comment.
I'm sure some state baseline tests probably could use this instead, but not worth the hassle right now.
|
Why don't we just enable allowJs for inferred projects without adjustments based on root files? |
|
We can do that, though I think we still need to force it on like Strada did, lest someone configure the inferred project manually, which is more or less the problem fourslash has. |
|
Oh, I'm dumb, the tests I'm referring to are tests which are explicitly trying to test behavior when allowJs is off. Maybe these tests are just bunk |
|
Here's an example of a fun one: --- old.goToDefinitionCSSPatternAmbientModule.baseline.jsonc
+++ new.goToDefinitionCSSPatternAmbientModule.baseline.jsonc
@@= skipped -1, +1 lines =@@
// === /index.css ===
// [||]html { font-size: 16px; }
-// === /types.ts ===
-// <|declare module [|"*.css"|] {
-// const styles: any;
-// export = styles;
-// }|>
-
// === /index.ts ===
// import styles from /*GOTO DEF*/[|"./index.css"|];We turn on both |
Good question—root files are only opened files, so if you only open |
Probably not. Any editors should probably set |
|
We actually do not yet support client setting any compiler options. |
|
Yeah, confirmed that |
|
Yeah, so this isn't really pressing to fix, thankfully. I'm going to look harder and see if it's possible for us to massage our current tests to use the defaults somehow. We do need to add support for the VS Code options that set these defaults, though. |
Fixes #2621
See also: #2621 (comment)
This required introducing a
@noDefaultOpenharness option, so that our fourslash runner doesn't open every file and cause our tests to not actually test "js exists but was not clicked into". The original fourslash runner worked directly on the language service without a server, so never hit this as there was no concept of "open".That being said, it's a little unclear how the server was working so well in JS code without this code?