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

feat(gatsby-plugin-sharp): Add tracedSVG option in fluid and fixed processors #11981

Merged
merged 2 commits into from
Feb 22, 2019

Conversation

oorestisime
Copy link
Contributor

@oorestisime oorestisime commented Feb 22, 2019

Fix: #11912

@wardpeet
Copy link
Contributor

wardpeet commented Feb 22, 2019

Mind adding some tests for it? It's good enough to just test if traceSVG is ran with the correct values. And not ran if it's not needed ^^
https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-plugin-sharp/src/__tests__/index.js

You also need to run yarn jest:update as your snapshots have not been updated. It happends because you have changed:
https://github.com/gatsbyjs/gatsby/pull/11981/files#diff-bccee37f4941978c8c573cc6663fb0b2R741
https://github.com/gatsbyjs/gatsby/pull/11981/files#diff-bccee37f4941978c8c573cc6663fb0b2R851

They aren't breaking so good to go 👍

@oorestisime
Copy link
Contributor Author

Oups sorry didn't pay attention to tests.

I also kind of avoided duplicate code from my first commit. i am not sure though if it is explicit enough. or maybe a better function name. let me know

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

Looks great overall! Requested a small change in a test assertion 🙂

args,
})

expect(result.tracedSVG).toBeUndefined()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a jest mock for the traceSVG function (it's already exported) and add an assertion that it's not called. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any way i could mock this properly?

thought about

const spy = jest.spyOn(rest, `traceSVG`)

    beforeEach(() => {
      spy.mockClear()
    })

    afterAll(() => {
      spy.mockClear()
    })

with rest

const {
  base64,
  fluid,
  fixed,
  queueImageResizing,
  getImageSize,
  ...rest
} = require(`../`)

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems trickier than I initially thought

Guess we should merge and iterate on the tests and the code (to make it easier to test) later

@sidharthachatterjee sidharthachatterjee changed the title Add tracedSVG option in fluid and fixed processors feat(gatsby-plugin-sharp): Add tracedSVG option in fluid and fixed processors Feb 22, 2019
@sidharthachatterjee sidharthachatterjee merged commit 8aaaa85 into gatsbyjs:master Feb 22, 2019
@sidharthachatterjee
Copy link
Contributor

Published in gatsby-plugin-sharp@2.0.22

wardpeet pushed a commit that referenced this pull request Feb 26, 2019
This is a follow-up on #11981 which addresses the following issue:

when user asks for tracedSVG sharp also calculates base64 image. This leads to longer build times and when using gatsby-image the base64 gets loaded before the tracedSVG.

Updated the unit-test that now shows that base64 should be undefined. let me know if you think i could improve this.
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.

3 participants