Skip to content

Syntax operations also need to ensure project is present for the open script infos since update could be pending to make sure open script info has project #50418

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

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

sheetalkamat
Copy link
Member

Also convert one relevant test case to baseline
Fixes #50131

… script infos since update could be pending to make sure open script info has project

Also convert one relevant test case to baseline
Fixes #50131
@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Aug 23, 2022
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.

Why would a project be required for outlining spans? Are there project-scoped settings that affect the behavior? Same question for (e.g.) brace matching.

@DanielRosenwasser
Copy link
Member

So what exactly happened before? Was it something like this?

  1. Open a file - a default project is automatically made.
  2. Close the file - the default project is automatically closed because it has no files open.
  3. Open another file - the default project is assumed not to be closed (?)

@sheetalkamat
Copy link
Member Author

So what exactly happened before?

  • Open a file say src/file.ts. Opens configured project say tsconfig.json
  • Close this file. Configured project is not collected because the removal of projects and scriptinfos that are orphan happens only when opening file
  • open another file say tests/file.ts It finds a tsconfig file tests/tsconfig.json but that does not include this file. But existing tsconfig.json has this file so we dont create inferred project. After this collecting orphan project collects tsconfig.json because there is no open file which will open this project (tests/file.ts does not count because any file in tests folder would open tests/tsconfig.json and not tsconfig.json) So now tests/file.ts is not part of any project
  • Call outlinging span and we were throwin error before the fix. With the fix it sees there is no project for this file so it creates inferred project for this file. (just like it would have if we didnt do first two steps)

@amcasey
Copy link
Member

amcasey commented Aug 23, 2022

I'm still missing why we'd need a project at all to handle this request. Is that just an unfortunate consequence of our design?

Comment on lines +1614 to +1621
const testsConfig: File = {
path: `${tscWatch.projectRoot}/playground/tsconfig.json`,
content: "{}"
};
const testsFile: File = {
path: `${tscWatch.projectRoot}/playground/tests.ts`,
content: `export function foo() {}`
};
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit of a nit - but really, is the idea that that playground contains some sort of implementation file, and playground/tests would contain the test file? In theory, you should only need 2 tsconfigs and 2 .ts files - but you don't need innerSrcFile, right?

src/
├── tsconfig.json
├── foo.ts
└── spec/
    ├── tsconfig.json
    └── foo.ts

Copy link
Member Author

Choose a reason for hiding this comment

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

This is replication of repro at #50131 (comment)

The playground/tsconfig.json is tests config i think or atleast the file opened was test file which imported the inner test file
The innerConfig is the implementation configuration but the test file resides next to it and isnt part of the config

Definitely not ideal configuration for sure. But the PR ensures that we dont crash and project is assigned.
We never look past first tsconfig.json we find because otherwise we would risk loading too many projects when opening file.

@sheetalkamat
Copy link
Member Author

I'm still missing why we'd need a project at all to handle this request. Is that just an unfortunate consequence of our design?

Thats our design for now.. in most cases the open files are already part of the project (the scenario we are fixing is just unfortunate) so it was never priority to experiment separate project for syntax only operations. (To get hold of LS on which we can forward those requests)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No Project error during getOutliningSpans
4 participants