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

Fix broken module resolution after large/rapid edits in nodenext #46818

Merged
merged 2 commits into from
Nov 17, 2021

Conversation

andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Nov 16, 2021

Fixes #46396

When updating a SourceFile, the incremental parser copies the old SourceFile’s impliedNodeFormat to the new one. Reusing module resolution from the old Program later expects that field to already be set on the updated SourceFile. However, the incremental parser is skipped when the language service gets overwhelmed by changes—specifically, when there are 8 or more changes logged against the snapshot before the language service asks for an updated SourceFile, or when the length of a text change is greater than 256 characters. In these cases, the SourceFile is re-parsed from scratch, and so it lacks its impliedNodeFormat field.

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Nov 16, 2021
@andrewbranch andrewbranch changed the title Fix broken module resolution after edits in nodenext Fix broken module resolution after large/rapid edits in nodenext Nov 16, 2021
@andrewbranch
Copy link
Member Author

A question now is whether to remove the field copying from the incremental parser. These should be redundant for all our own uses, but it’s possible someone could use the incremental parser standalone. Doing it there is valid, but copying those kinds of fields is not generally the job of the incremental parser 🤷

@andrewbranch
Copy link
Member Author

@weswigham thanks for the review—any opinion on whether the incremental parser should still copy that field? I’m fairly happy to err on the side of leaving it there, as I don’t think it’s hurting anything.

@weswigham
Copy link
Member

The incremental parser copies/reparses things like the comment directive list onto the new source file - I think it's fine for it to also copy the impliedNodeFormat field. The parser can't add it on it's own because you need either a module resolution host or an old source file to get the information, so it's kinda the first place that information is available during an incremental reparse.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Nov 17, 2021

So is it wrong to do it in program.ts? Do we need both? I'm mostly asking for my own understanding.

@weswigham
Copy link
Member

I don't think there's a right or wrong place - this field has some pretty unique requirements among SourceFile members because of everything that goes into calculating its initial value - we just set it whenever we can.

@andrewbranch andrewbranch merged commit f11f14b into microsoft:main Nov 17, 2021
@andrewbranch andrewbranch deleted the bug/46396 branch November 17, 2021 21:04
mprobst pushed a commit to mprobst/TypeScript that referenced this pull request Jan 10, 2022
…rosoft#46818)

* Fix broken module resolution after edits in nodenext

* Move field copying to a better place I guess
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.

nodenext alternates between finding and not finding the imported package
4 participants