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

Handle when default project for file is solution with file actually referenced by one of the project references #37239

Merged
merged 11 commits into from
Mar 13, 2020

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Mar 6, 2020

Fixes #36708, #27372

Now when we find that the project is a solution project, we try to open referenced projects to see if we can find project that contains the opened file.

@amcasey
Copy link
Member

amcasey commented Mar 6, 2020

A solution project is a composite project with references to other projects, but no files of its own?

@uniqueiniquity
Copy link
Contributor

@amcasey That appears to be the case, but I would appreciate confirmation as well. Also, how common is that as a pattern?

@sheetalkamat
Copy link
Member Author

A solution project is project with project referenes but no files on its own.. (Eg https://github.com/microsoft/TypeScript/blob/master/src/tsconfig.json)

@sheetalkamat
Copy link
Member Author

Note that we already handle solutions for scenarios like our own repo.. Wherein the tsconfig for compiler is in its own folder and named tsconfig.json so found for the file open in src\compiler ... This tries to solve cases if say that tsconfig was named tsconfig.compiler.json or was in src\compiler or src itself instead. Good repro scenario to look at would be the issues this PR is fixing.. #27372 has a link to https://github.com/OliverJAsh/jest-ts-project-references which demos the issue.

@DanielRosenwasser
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 10, 2020

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/67599/artifacts?artifactName=tgz&fileId=ED6224134493FAA69B7797634F6182922F217A4923B3181D8B91B238C9CBB63002&fileName=/typescript-3.9.0-insiders.20200310.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

How did you identify the code paths that needed to be updated (i.e. how do you know you didn't miss one)?

src/server/editorServices.ts Show resolved Hide resolved
src/server/editorServices.ts Show resolved Hide resolved
src/server/editorServices.ts Show resolved Hide resolved
src/server/editorServices.ts Show resolved Hide resolved
@sheetalkamat
Copy link
Member Author

How did you identify the code paths that needed to be updated (i.e. how do you know you didn't miss one)?

Well i looked at all the calls to getConfigFileNameForFile which is where after finding solution project, we are changing the behavior

@amcasey
Copy link
Member

amcasey commented Mar 13, 2020

How did you identify the code paths that needed to be updated (i.e. how do you know you didn't miss one)?

Well i looked at all the calls to getConfigFileNameForFile which is where after finding solution project, we are changing the behavior

It sounds like anyone who calls getConfigFileNameForFile in the future will need to think about this as well. Is there some way we could organize the code to guard against a new caller failing to do so? Perhaps it could become a private helper of findConfiguredProjectByProjectName, for example?

@amcasey
Copy link
Member

amcasey commented Mar 13, 2020

Confirmed that #36708 and #27372 no longer repro. The tests work better if I launch the version of VS Code running against this change. 😝

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

I'm mildly concerned about missing a case now or in the future, but this looks correct to the extent of my ability to verify it.

@kripod
Copy link

kripod commented Apr 5, 2020

Can a solution tsconfig.json file contain solution-wide compilerOptions? Is the configuration below valid?

// tsconfig.json
{
  "files": [],
  "references": [
    { "path": "./packages/client/tsconfig.json" },
    { "path": "./packages/server/tsconfig.json" },
  ],
  "compilerOptions": {
    "noEmit": true,
    "allowJs": true,
    "checkJs": true,
    "strict": true,
    "target": "ES2019",
    "module": "ESNext",
    "moduleResolution": "Node",
    "esModuleInterop": true,
    "isolatedModules": true,
    "jsx": "react"
  }
}
@amcasey
Copy link
Member

amcasey commented Apr 6, 2020

@kripod I think you're asking whether your client and server projects will automatically pick up those compiler options - no, they won't. If you want to inherit the configuration, you need to use extends in the derived/child projects.

@martaver
Copy link

Is this patch available in typescript@dev or typescript@next...?

If so, then the use case described in: kulshekhar/ts-jest#1698 isn't resolved by this PR.

This is relevant because ts-jest is dependent on this API for resolving modules that are project references, and if this PR is supposed to fix that - it hasn't.

@sheetalkamat
Copy link
Member Author

@martaver I am not sure what you are asking here. i cloned the repo, checkout branch and opened vscode and that seems to work without reporting errors.

The build steps do not use tsc so it would be hard to tell whats going on. you would need to seek help from the package that is building it.

image

@ahnpnl
Copy link

ahnpnl commented Jul 26, 2020

Hi, I want to implement the same behavior programmatically with LanguageService interface but I am kind of lost when reading this PR. Do you have any help documentation to implement similarly ?

@aleclarson
Copy link

aleclarson commented Nov 9, 2020

https://github.com/microsoft/TypeScript/blob/06a2210eb53cfce968dc855fcc13738d95d4556a/src/tsconfig.json

How are "solution projects" useful in that example? Doesn't the language server look for the nearest tsconfig.json before compiling a module (by checking each ancestor, bottom-up)? In that example, the "solution project" seems superfluous.

PS: We would appreciate a response to #33407. I'd prefer to use an overrides key that can target specific globs, without the need to create a separate .json module.

{
	"include": ["src"],
	"compilerOptions": {
		// These options are inherited by override globs.
		"moduleResolution": "node",
		"strict": true 
	},
	"overrides": [
		{
			"include": ["**/__tests__"],
			"compilerOptions": {
				"types": ["jest", "node"]
			}
		}
	]
}
@aleclarson
Copy link

I've tried this approach without success. See anything wrong in this gist?
https://gist.github.com/aleclarson/6ae9a3b6b9d06c492fb9a3b6ec395411

ahuth added a commit to chanzuckerberg/frontend-libs that referenced this pull request Nov 17, 2020
Note that without extending the top-level tsconfig.json from tsconfig.base.json, the right compiler options weren't being applied for the tests, even though they are inherited by the individual project tsconfig.jsons (react and esModuleInterop).

Extending the top level one from the base solves the issue, but feels wrong... I would've thought that because each project gets the right config, the tests should've worked right. Also building works fine, but it is Jest or ts-jest causing the issue.

Pretty sure this is related to
- kulshekhar/ts-jest#1648
- microsoft/TypeScript#37239
ahuth added a commit to chanzuckerberg/frontend-libs that referenced this pull request Nov 17, 2020
Note that without extending the top-level tsconfig.json from tsconfig.base.json, the right compiler options weren't being applied for the tests, even though they are inherited by the individual project tsconfig.jsons (react and esModuleInterop).

Extending the top level one from the base solves the issue, but feels wrong... I would've thought that because each project gets the right config, the tests should've worked right. Also building works fine, but it is Jest or ts-jest causing the issue.

Pretty sure this is related to
- kulshekhar/ts-jest#1648
- microsoft/TypeScript#37239
ahuth added a commit to chanzuckerberg/frontend-libs that referenced this pull request Nov 18, 2020
Note that without extending the top-level tsconfig.json from tsconfig.base.json, the right compiler options weren't being applied for the tests, even though they are inherited by the individual project tsconfig.jsons (react and esModuleInterop).

Extending the top level one from the base solves the issue, but feels wrong... I would've thought that because each project gets the right config, the tests should've worked right. Also building works fine, but it is Jest or ts-jest causing the issue.

Pretty sure this is related to
- kulshekhar/ts-jest#1648
- microsoft/TypeScript#37239
@rattrayalex
Copy link

I also could not get this to work, with roughly this setup:

// tsconfig.base.json
{
  "compilerOptions": {"strictNullChecks": true}
}

// tsconfig.js.json
{
  "extends": "./tsconfig.base.json",
  "include": ["**/*.jsx?"],
  "compilerOptions": {"checkJs": true, "allowJs": true, "noImplicitAny": false}
}

// tsconfig.ts.json
{
  "extends": "./tsconfig.base.json",
  "include": ["**/*.tsx?"],
}

// tsconfig.json
{
  "files": [],
  "references": [
    { "path": "./tsconfig.js.json" },
    { "path": "./tsconfig.ts.json" }
  ]
}

VS Code behaved as if not tsconfig were detected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment