Skip to content

fix: add a temporary property '_isRootFile_' for each promise result of cache to prevent i…#165

Merged
dominikg merged 2 commits intodominikg:mainfrom
daihere1993:fix/deadlock_pr
Mar 8, 2024
Merged

fix: add a temporary property '_isRootFile_' for each promise result of cache to prevent i…#165
dominikg merged 2 commits intodominikg:mainfrom
daihere1993:fix/deadlock_pr

Conversation

@daihere1993
Copy link
Copy Markdown
Contributor

I've just came across the issue of vitest-dev/vitest#5182. Then found out the RC is #154. So I spent some times to read relevant codes and put my correction out to try to push this issue forward. The tests based on #157.

My considerations about this fix: this fix is based on the current design and as few changes as possible to just add a patch to prevent into the deadlock situation you've described in aleclarson/vite-tsconfig-paths#132 (comment).

In the correction, I add an internal property _isRootFile_ for each cached file and this new property means if the file is invoked by the public api parese() which used to distingust another type of cache which created by parseFile(). Then using _isRootFile_ prevent into the deadlock in parseFile().

@dominikg let me know if you have some comments.

@dominikg
Copy link
Copy Markdown
Owner

dominikg commented Mar 3, 2024

great, will have a closer look soon. is it possible to hide the flag with a non enumerable ?

@daihere1993
Copy link
Copy Markdown
Contributor Author

daihere1993 commented Mar 4, 2024

great, will have a closer look soon. is it possible to hide the flag with a non enumerable ?

Done.

By the way, this flag is just bound to the "promise result" would not exist in the result returned to user, it's more like a temporary flag druing the parse process.

@dominikg
Copy link
Copy Markdown
Owner

dominikg commented Mar 5, 2024

Thanks! looks like you also have to run pnpm generate and commit the changes.

The reason for hiding is that the promise can be accessed from the cache. Yes its temporary and users shouldn't enumerate the promise, but better safe than sorry ;)
The changes to the cache test look clean to 🌈 Have to see if they fit future plans though. The complete eager parsing of referenced is hurting performance, so tsconfck 4.0 might change that to lazy referenced parse.

see also #162 that would be a breaking change so it didn't move forward yet

@daihere1993
Copy link
Copy Markdown
Contributor Author

CI passed now.
Yeah, to persue better performance then we need some refactor. I'll have a look of #162 and would like try to have another PR about speed imprevement.

@daihere1993 daihere1993 changed the title fix: add internal property '_isRootFile_' for each cache to prevent i… fix: add temporary property '_isRootFile_' for each promist result of cache to prevent i… Mar 5, 2024
@daihere1993 daihere1993 changed the title fix: add temporary property '_isRootFile_' for each promist result of cache to prevent i… fix: add a temporary property '_isRootFile_' for each promist result of cache to prevent i… Mar 5, 2024
@daihere1993 daihere1993 changed the title fix: add a temporary property '_isRootFile_' for each promist result of cache to prevent i… fix: add a temporary property '_isRootFile_' for each promise result of cache to prevent i… Mar 5, 2024
@dominikg
Copy link
Copy Markdown
Owner

dominikg commented Mar 8, 2024

added a changeset and made the flag readonly, planning to release a patch with it soon.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants