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(webpack): add cacheProvider to support customized cache #884

Merged
merged 1 commit into from
Dec 17, 2021
Merged

fix(webpack): add cacheProvider to support customized cache #884

merged 1 commit into from
Dec 17, 2021

Conversation

malash
Copy link
Contributor

@malash malash commented Dec 13, 2021

Motivation

This PR try to fix #881 . This PR is mainly inspired from a281440

Summary

This PR add a new option cacheProvider for Linaria loader.

  • cacheProvider: undefined | string | ICache (default: undefined):
    By default Linaria use a memory cache to store temporary CSS files. But if you are using this loader with thread-loader you should use some consistent cache to prevent some unexpected issues. This options support a ICache instance or a path to NodeJS module which export a ICache instance as module.exports
    interface ICache {
      get: (key: string) => Promise<string>;
      set: (key: string, value: string) => Promise<void>
    }
    

Test plan

I run yarn build in this repo and copied libs folder to my Linaria project's node_modules/linaria folder.

@Anber
Copy link
Collaborator

Anber commented Dec 14, 2021

Hi @malash!
Thank you for the pull request!

By pulling back filesystem-based cache we are pulling back all the problems with detecting the project root and the cache directory. I'm thinking… maybe we can just introduce a cache option, instead of cacheType? It can be an implementation of a simple interface like interface ICache { get: (key: string) => string; set: (key: string, value: string) => void }. The default implementation will be an empty Map, but it can be replaced with whatever you want: mock, filesystem or even Redis (if someone wants to use distributed build :)). What do you think?

@malash
Copy link
Contributor Author

malash commented Dec 14, 2021

@Anber Good idea.

I think the interface should be async. Because async fs provide better performance than sync fs, and Redis might only support async API. And to implement the feature, the Webpack loader should be refactored with async loader.

interface ICache { 
  get: (key: string) => Promise<string>;
  set: (key: string, value: string) => Promise<void>
}.

If ok, I can refactor the PR.

@malash
Copy link
Contributor Author

malash commented Dec 15, 2021

@Anber I realized there might be another problem if we chose the ICache plan.

In my case I want to use Linaria with thread-loader, and thread-loader always serialize the loader context for cross-thread communication. Unfortunately functions cannot be serialized, so this plan still cannot solve the problem #881 .

How about implementing the cache option like this:

cache: 'memory' | 'filesystem' | ICache;

It should be scalable enough ( even someone want to use Redis-based distributed build ).

@Anber
Copy link
Collaborator

Anber commented Dec 15, 2021

cache: 'memory' | 'filesystem' | ICache;

It still requires find-yarn-workspace-root and other hacks…
Maybe instead of cache implementation, we can specify a path to a file/module with implementation? It would be serializable.

@malash
Copy link
Contributor Author

malash commented Dec 17, 2021

@Anber I refactored the PR, could you please take a look?

I tested this branch in my project and it works.

@malash malash changed the title fix(webpack): add cacheType to support both memory and filesystem cache fix(webpack): add cacheProvider to support customized cache Dec 17, 2021
@Anber
Copy link
Collaborator

Anber commented Dec 17, 2021

@malash thank you for your awesome work!

@Anber Anber merged commit 1e39317 into callstack:2.0.x Dec 17, 2021
@Anber
Copy link
Collaborator

Anber commented Dec 17, 2021

I'll publish it as 2.3 later today.

@Anber
Copy link
Collaborator

Anber commented Dec 17, 2021

Released

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.

None yet

2 participants