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(*): cache tracedSVG calculations when cache is present #12044

Merged
merged 4 commits into from
May 28, 2019

Conversation

oorestisime
Copy link
Contributor

Fix: #11997

Similar to #9059 but for tracedSVG.

@wardpeet
Copy link
Contributor

sweet, mind adding some tests to make sure your cachekeys are working

@oorestisime
Copy link
Contributor Author

hey, thanks for the feedback! I am not sure i follow here.
Is there a test somewhere doing this? Doesn't sound good to me to export the cache key generator.
But i might be seeing this completely wrong so a few more details would be appreciated!

@wardpeet
Copy link
Contributor

wardpeet commented Mar 20, 2019

oh sorry, more or less I mean if we call base64 twice it is only ran once.

An even better way is to mock sharp and see it only been called once but that might be not so trivial.

it(`caches base64 result`, async () => {
      const result = await base64({
        file,
        args,
      })
      const result2 = await base64({
        file,
        args,
      })

      expect(result).toMatchSnapshot()
      // it needs to be a === equal check
       expect(result).toBe(result2)
    })

@oorestisime
Copy link
Contributor Author

Heya,

doing so would produce the same result but without actually testing cache keys. I won't be able to pass a cache object in the arguments since there is no way to import the cache as a standalone (i think).
Only solution i see here is mocking the cache object, although it seems that doing so beats the purpose of the test

@LekoArts
Copy link
Contributor

Would be cool to have this in! Any follow-up questions/additions from your side @wardpeet ?

@oorestisime
Copy link
Contributor Author

@wardpeet kind reminder :)

@wardpeet
Copy link
Contributor

wardpeet commented May 24, 2019

@oorestisime oh my bad. It looks great, can you fix conflicts and we'll merge! 👍

@oorestisime
Copy link
Contributor Author

Rebased :) please have a close look i hope i did a correct merge, there were a lot of changes

@wardpeet
Copy link
Contributor

Patched a few things. I'll be testing this one and getting this in. Thanks for your patience and rebasing it!

@wardpeet wardpeet changed the title Cache tracedSVG calculations when cache is present fix(*): cache tracedSVG calculations when cache is present May 27, 2019
@wardpeet wardpeet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label May 27, 2019
@oorestisime
Copy link
Contributor Author

Yeah sorry i did this quickly because i ll be with no pc for a week so i didn't want to get it stalled! thanks for taking over !

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Let's merge @oorestisime thanks for your work and patience!

@wardpeet wardpeet merged commit c40bc4b into gatsbyjs:master May 28, 2019
@gatsbot
Copy link

gatsbot bot commented May 28, 2019

Danger run resulted in 1 fail; to find out more, see the checks page.

Generated by 🚫 dangerJS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[gatsby-plugin-sharp] Cache traceSVG calculations
3 participants