Skip to content

fix: import cache#259

Merged
ai merged 5 commits intopostcss:mainfrom
PengBoUESTC:patch-1
Feb 8, 2024
Merged

fix: import cache#259
ai merged 5 commits intopostcss:mainfrom
PengBoUESTC:patch-1

Conversation

@PengBoUESTC
Copy link
Copy Markdown
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
Copy Markdown
Member

ai commented Jan 30, 2024

@brc-dd do you think it is safe?

@brc-dd
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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

try {
return (await import(pathToFileURL(url).href)).default
return (await import(`${pathToFileURL(url).href}?t=${Date.now()}`)).default
Copy link
Copy Markdown
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
Copy Markdown
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
@ai
Copy link
Copy Markdown
Member

ai commented Feb 8, 2024

Thanks. Released in 5.0.3.

@PengBoUESTC
Copy link
Copy Markdown
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