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

Improve performance by avoid fetching unnecessary files #33

Closed
wants to merge 4 commits into from

Conversation

khanguy00
Copy link

@khanguy00 khanguy00 commented May 8, 2021

Resolves #32

Approach

When walking into a CSS, the plugin will do 2 things separately:

  1. Collect definitions @value.
  2. Resolve all definitions when needed.

Weakness

  • If we have 2 import statements that refer to the same CSS file, the plugin will walk into that file twice.

@khanguy00
Copy link
Author

The newest commit 7cbe827 now integrates the caching idea from #34 to make this process potentially faster.

The idea of caching is:

  1. We cache the result in walkFile to avoid walking the same file again (same as Cache file definitions (#34), closes #32 #34, thanks to @WearyMonkey)
  2. In the cached result, there may be the resolved value or the raw definition
  3. The next time we hit the same file, we retrieve the required definitions from the cache and re-evaluate if needed.

@princed
Copy link
Owner

princed commented May 10, 2021

Thank you @khanguy00 and @WearyMonkey, I'll try and have a look later today.
Does it mean we can close #34?

@khanguy00
Copy link
Author

Does it mean we can close #34?

I think so. Unless you're about to decline this PR and accept that instead. 😂

@princed
Copy link
Owner

princed commented May 17, 2021

I've decided to go with Toby's solution as it's much simpler and still solves the problem. Please feel free to reopen if/when caching isn't enough. Thank you!

@princed princed closed this May 17, 2021
@WearyMonkey
Copy link
Contributor

@khanguy00 it might be interesting to compare your change against the baseline of my change. If there is an improvement, then it might be worth submitting this as a follow on fix.

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.

Ignore unused values to boost performance
3 participants