postcss / postcss-load-config

Autoload Config for PostCSS
MIT License
638 stars 71 forks source link

fix: import cache #259

Closed PengBoUESTC closed 5 months ago

PengBoUESTC commented 6 months ago

Notable Changes

Commit Message Summary (CHANGELOG)

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

Type

SemVer

Issues

https://github.com/vitejs/vite/issues/15745

Checklist

ai commented 6 months ago

@brc-dd do you think it is safe?

brc-dd commented 6 months ago

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 https://github.com/nodejs/node/issues/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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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.

ai commented 6 months ago

I need some proof that it will not make everything worse.

For instance, that some big projects use the same approach.

brc-dd commented 6 months ago

Astro has this - https://github.com/withastro/astro/blob/b956149b2805fdd977d20e267f05c6c4872e2704/packages/astro/src/core/config/vite-load.ts#L45

Nx has - https://github.com/nrwl/nx/blob/09abd39ecc1e11ef94b404d4512fca3526d20d31/packages/remix/src/utils/remix-config.ts#L52 / https://github.com/nrwl/nx/blob/09abd39ecc1e11ef94b404d4512fca3526d20d31/packages/remix/src/plugins/plugin.ts#L189

Sindre's repo has this way mentioned - https://github.com/sindresorhus/import-fresh?tab=readme-ov-file#esm

Directus - https://github.com/directus/directus/blob/e888ba8112384b950723b128d7e4642ad8bc8d7b/api/src/utils/import-file-url.ts#L11

ai commented 5 months ago

Thanks. Released in 5.0.3.

PengBoUESTC commented 5 months ago

Thanks. Released in 5.0.3.

❤ī¸