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: import cache #259

Merged
merged 5 commits into from
Feb 8, 2024
Merged

fix: import cache #259

merged 5 commits into from
Feb 8, 2024

Conversation

PengBoUESTC
Copy link
Contributor

Notable Changes

Commit Message Summary (CHANGELOG)

add query for import file path, to avoid import cache.

Type

  • CI
  • Fix
  • Perf
  • Docs
  • Test
  • Chore
  • Style
  • Build
  • Feature
  • Refactor

SemVer

  • Fix (:label: Patch)
  • Feature (:label: Minor)
  • Breaking Change (:label: Major)

Issues

vitejs/vite#15745

  • Fixes #1

Checklist

  • Lint and unit tests pass with my changes
  • I have added tests that prove my fix is effective/works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes are merged and published in downstream modules

@ai
Copy link
Member

ai commented Jan 30, 2024

@brc-dd do you think it is safe?

@brc-dd
Copy link
Contributor

brc-dd commented Jan 30, 2024

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 JITI_CACHE env variable to customize this behavior, but for native imports, I guess it's fine to bypass caching.

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.

@benjaminpreiss
Copy link

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)

@brc-dd
Copy link
Contributor

brc-dd commented Jan 30, 2024

I'd like to understand this better

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.

@benjaminpreiss
Copy link

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. This behavior is same on deno.

I see - Is there no garbage collection active on unused modules in the module cache?

@brc-dd
Copy link
Contributor

brc-dd commented Jan 30, 2024

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.

@PengBoUESTC PengBoUESTC requested a review from brc-dd January 31, 2024 03:05
@ai
Copy link
Member

ai commented Feb 1, 2024

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
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@PengBoUESTC PengBoUESTC requested a review from brc-dd February 2, 2024 01:45
@ai ai merged commit 3ade20e into postcss:main Feb 8, 2024
3 checks passed
@ai
Copy link
Member

ai commented Feb 8, 2024

Thanks. Released in 5.0.3.

@PengBoUESTC
Copy link
Contributor Author

Thanks. Released in 5.0.3.

❤️

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.

4 participants