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

Don't run base64 calculation when demanding tracedSVG #12043

Merged
merged 2 commits into from
Feb 26, 2019

Conversation

oorestisime
Copy link
Contributor

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.

@wardpeet
Copy link
Contributor

Why shouldn't we calculate the base64? Is it because we won't use it anyway as tracedSVG is used as a placeholder image?

@oorestisime
Copy link
Contributor Author

Yeah when user opts in for tracedSVG then he will want to use that as initial image.

I would guess this also happens in the gatsby-transformer-sharp but i haven't really checked. There's an option base64: false which i think most people use when using tracedSVG. here's some more context oorestisime/gatsby-remark-rehype-images#2 (comment)

@wardpeet
Copy link
Contributor

Well, this means that this would be a breaking change. Couldn't we just introduce base64: false for fluid and fixed and document it. Also, add a TODO that we want to remove this behaviour in gatsby V3 and by default set base64 to false when tracedSVG is used.

@wardpeet wardpeet added breaking change If implemented, this proposed work would break functionality for older versions of Gatsby status: awaiting author response Additional information has been requested from the author labels Feb 25, 2019
@oorestisime
Copy link
Contributor Author

What do you mean a breaking change? Generating tracedSVG placeholders in plugin-sharp during fixed and fluid processing was added like two days ago.
I mean ok it is a breaking on the paper but i am not sure users opted in already and if they did they would have found the behavior problematic as well no? I am probably missing something here!

@wardpeet
Copy link
Contributor

no no, the breaking change is removing base64 when tracedSVG is true. Now it generates base64 always even before your fix from 2 days ago.

@oorestisime
Copy link
Contributor Author

oorestisime commented Feb 25, 2019

But before the fix it wasn't generating tracedSVG at all during fluid or fixed processing :D any way we can discuss this on discord or somewhere more live? i am probably missing something!

@KyleAMathews
Copy link
Contributor

I'm not sure I understand this change. Is the problem that base64 is being generated even when it's not needed?

@oorestisime
Copy link
Contributor Author

Yes, and then in gatsby-image it gets loaded https:/gatsbyjs/gatsby/blob/master/packages/gatsby-image/src/index.js#L280-L288 even if we ask for tracedSVG.
this will not impact gatsby-transformer-sharp since it is generating tracedSVG by calling directly plugin-sharp and it does not pass the generateTracedSVG option

@KyleAMathews
Copy link
Contributor

gatsby-image only renders what's passed into it. If a query doesn't ask for base64, it's not loaded.

Check our the tracedSVG fragments. They're not querying the base64 field https:/gatsbyjs/gatsby/blob/master/packages/gatsby-transformer-sharp/src/fragments.js

@oorestisime
Copy link
Contributor Author

Yeah right but this happens when you can control the query arguments. When another plugin uses fluid or fixed and asks for tracedSVG then base64 would anyway be there since it is generated.

A solution would be to just delete base64 on the third party end but i find this hacky!
Here's the plugin in question https:/oorestisime/gatsby-remark-rehype-images/blob/master/src/index.js it parses custom markdown tags and process the images. Combined with rehypeReact you can get the gatsby-image goodies in simple markdown format.

I am just not seeing any breaking change :/ When user asks fluid or fixed for tracedSVG doesn't it make sense not to generate base64? This will not impact gatsby-transformer-sharp. Unless i am really missing something here, which can totally be the case!

@oorestisime
Copy link
Contributor Author

Discussed on discord. current implementation is confusing since i am changing options based on another option.
so i will add a new option base64: false and tracedSVG users will need to pass this as well.

In a breaking change we could add placeholderImageType: [tracedSvg, base64]. i guess this would need an issue tagged breaking change and should be done when v3 is on the works?

@wardpeet wardpeet removed the breaking change If implemented, this proposed work would break functionality for older versions of Gatsby label Feb 26, 2019
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.

LGTM! thanks for updating this one!

@wardpeet wardpeet merged commit 60cbc48 into gatsbyjs:master Feb 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting author response Additional information has been requested from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants