-
-
Notifications
You must be signed in to change notification settings - Fork 73
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: import cache #259
fix: import cache #259
Conversation
@brc-dd do you think it is safe? |
Yeah. It's quite standard way to avoid caching. For most cases, it won't affect anything (as generally every tool calls postcss-load-config only once or twice). But for test runners that don't properly isolate stuff, this is needed if you're writing/reading config from same paths. Jiti supports OP can probably explain what exactly their use-case is. There is a memory leak associated with this though (refer comments in nodejs/node#49442). But those are hard to avoid, and like I said since tools don't load postcss config too many times, it should fine. |
I'd like to understand this better (as I have a usecase where the postcss.config.js would reload quite often) |
From what I understand, node's module cache has circular references and gc cannot exactly know if a particular module is no longer used. With this PR, each call to postcss-load-config will make node treat the config as a separate module, and the module cache will grow over time. From my tests, the memory leak seems to be there in deno and bun as well. Without this PR, each call to postcss-load-config will return the same config even if the config has changed. |
I see - Is there no garbage collection active on unused modules in the module cache? |
Yeah, node/deno/bun currently don't remove anything from the module cache, nor provide any way to do that. |
I need some proof that it will not make everything worse. For instance, that some big projects use the same approach. |
src/req.js
Outdated
@@ -16,7 +16,7 @@ async function req(name, rootFile = __filename) { | |||
let url = createRequire(rootFile).resolve(name) | |||
|
|||
try { | |||
return (await import(pathToFileURL(url).href)).default | |||
return (await import(`${pathToFileURL(url).href}?t=${Date.now()}`)).default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.href can be removed, template literal will call toString by default which returns href
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Thanks. Released in 5.0.3. |
❤️ |
Notable Changes
Commit Message Summary (CHANGELOG)
Type
SemVer
Issues
vitejs/vite#15745
#1
Checklist