chore(*): bump of sharp for Node v12 compatibility

[eclectic-coding]

Description

Version bump of sharp in gatsby-plugin-sharp for Node v12 compatibility. Fixes #13635

[pieh]

This turns out to be more problematic than I thought - we already tried bumping sharp once (in one of the plugin) and reverted it ( #13271 ). Seems like there are some problems when multiple versions of sharp are being used. So we probably need to update sharp in every package that uses it.

The other problem is that even if we do update it here all at once. Users might update just one of those and will hit similar problems, so we do need a way to handle that (it's not yet clear what's best approach, but my initial thought is to check versions of sharp that are installed, and if there is more than one, suggest updating all packages that depend on sharp)

[eclectic-coding]

@pieh Looking through the ci/circleci tests errors, it looks like packages/gatsby-plugin-sharp/src/__tests__/index.js is failing around line 14. Specifically, queueImageResizing.

[eclectic-coding]

Definitely looks more complete. Thanks for reviewing.

[pieh]

Even if my commit fixes the tests - the PR is still blocked by not handling mismatched sharp versions

[KyleAMathews]

but my initial thought is to check versions of sharp that are installed, and if there is more than one, suggest updating all packages that depend on sharp)

👍 we could just add this to core I think

[moonmeister]

@pieh if we're updating all sharp versions why is this blocked? I'm not understanding.

[pieh]

@pieh if we're updating all sharp versions why is this blocked? I'm not understanding.

Because if user will update just single package that depend on sharp he will hit error like:

➜  gatsby-starter-blog git:(master) ✗ gatsby build
success open and validate gatsby-configs — 0.009 s
error 
Something went wrong installing the "sharp" module

dlopen(/Users/misiek/dev/gatsby-starter-blog/node_modules/sharp/build/Release/sharp.node, 1): Library not loaded: @rpath/libglib-2.0.dylib
  Referenced from: /Users/misiek/dev/gatsby-starter-blog/node_modules/sharp/build/Release/sharp.node
  Reason: Incompatible library version: sharp.node requires version 6001.0.0 or later, but libglib-2.0.dylib provides version 5801.0.0

- Remove the "node_modules/sharp" directory, run "npm install" and look for errors
- Consult the installation documentation at https://sharp.pixelplumbing.com/en/stable/install/
- Search for this error at https://github.com/lovell/sharp/issues

You can try it for yourself by creating new project from gatsby-starter-blog and pinning gatsby-plugin-manifest to 2.0.28 (this release had sharp bumped to 0.22. In this scenario gatsby-plugin-sharp, etc were still on sharp@0.21, which caused mismatch and sharp binaries failing to load

[eclectic-coding]

As i posted on twitter this weekend, the goal should be to have this resolved before Node v.12 moves to LTS in October, 2019. Gatsby seems to work fine with v. 10.15 LTS, and v.11.x.x.

[pieh]

I opened issue/question in sharp repo ( lovell/sharp#1680 ), so let's hold on a bit with implementing workaround to see what Lovell thinks of it. If it can't be fixed / is not feasible - then we'll go ahead with doing this extra check to make sure only single sharp version is being installed

[eclectic-coding]

Thank you @pieh
I agree to the slow down approach. After all this only seems to affect v. 12 of Node which was released just last week. We have some time before it becomes the default LTS version, as long as we are addressing for those users which use the latest versions.
Thanks again.

[pieh]

As mentioned in #13781 (comment) adding warning seems really unfeasible, so best way forward is to document how to troubleshoot this (hopefully section I wrote would be easily google'able and would come up as top result after searching for the exact error)

[pieh]

One note - I added troubleshooting section to one plugin (for now). And I plan to copy it over to rest of the plugins docs, which I would do after review/feedback

[KyleAMathews]

With the Ink change Ward is working on, couldn't we detect certain errors and show a help message? Obviously that's not ready yet so just thinking through options.

Also is there a cheap way to ask node what versions of a module are resolvable? We could detect mismatches this way.

[pieh]

With the Ink change Ward is working on, couldn't we detect certain errors and show a help message? Obviously that's not ready yet so just thinking through options.

This relies on user having updated Gatsby core (if we would implement it), which is the problematic scenario - user add or updates single package without updating all of them.

Also is there a cheap way to ask node what versions of a module are resolvable? We could detect mismatches this way.

We can try npm list sharp and yarn why sharp and parse the output (or maybe find package that already abstracts this). But the question is where do we implement it? Do we implement this in core? (same problem as above). Do we implement this in those plugins? It may actually not catch the problem, because if user update one of the packages and module from the package that has this handling is parsed/exucuted first, at this point in runtime there is no error), if next packages use different sharp version this is when error will occur, and because user potentially didn't update those there will be no code to handle that. This is what I meant that this might not be feasible to do.

We could try to take this upstream and add instructions directly in sharp (similar to what @sidharthachatterjee did for general sharp installation problems), but then instructions will need to be very abstract and not target Gatsby plugins.

[pieh]

Ok, I think I can scratch my previous comment as it wasn't entirely true it seems - the problem only occur when older sharp version was loaded first - other way around it works (so for some users - depending on order of their plugins in gatsby-config - they might use different version of sharp without problems).

But luckily thanks to that, we can actually catch problematic error (when newer sharp is loaded later).

I pushed WIP of sharp error message decorated with Gatsby specific instructions. Those instructions are still TO-DO, but we do get list of packages that depend on sharp along with sharp version, so the message should mention to update them (if npm list will work, I have to low sample size to be totally confident in that).

In any case this is output using that code:

[pieh]

Updated version of error message:

And less specific version (in case grabbing and parsing list of packages with npm list fails:

[wardpeet]

I like it! I'm unsure if we should show the original error but the advice is 👌

feel free to merge.

[pieh]

@sidharthachatterjee just mentioned that checking error message when determining if we should show message might be too naive, investigating it now (i.e. see what error looks like on ubuntu and windows - it seems to me like it may be platform specific error)

[pieh]

Soooo, this error happen only on OSX - Ubuntu and Windows work just fine :O

[pieh]

I like it! I'm unsure if we should show the original error but the advice is 👌

I was considering hiding it, but because of general hackiness of this code I thought that not swallowing original message is safer option (in case Gatsby specific advice doesn't work or apply, user have more feedback to try to find solution or create issue about it)

[sidharthachatterjee]

Looking good!

Left a few grammar suggestions and a few questions

[sidharthachatterjee]

Does this factor in a lock file? Would this be inconsistent in case there are both lock files?

[pieh]

good point, it will make use of package-lock.json if it's available and potentially show not accurate results. If lock file doesn't exist, it will traverse node_modules. This makes it very hard to support, but I think maybe opting out of it if both yarn and npm lock files exist seems reasonable?

[sidharthachatterjee]

Note: Remember to add safe-sharp to all instances in all packages that depend on it

[pieh]

Hey @polishedwp, could you commit suggestions made by @sidharthachatterjee? It seems like I can't do it because it's not my PR :)

[KyleAMathews]

Looks beautiful!

[eclectic-coding]

Well it says I am not authorized either.

[DSchau]

@polishedwp I think @pieh was requesting you apply the suggestions, not merge the PR!

[eclectic-coding]

@DSchau Ok, I think I have now.

[pieh]

Sorry for confession @polishedwp :(

There are couple of suggestions left - to get them all at once you can go to https://github.com/gatsbyjs/gatsby/pull/13646/files , scroll through code changes, use "Add sugestion to batch" button in suggestion UI and when all are added to batch, on top there is "Commit suggestions" to actually create commit with all the changes

I really appreciate it and once more, sorry for inconvenience.

[eclectic-coding]

This should resolve the commits

[eclectic-coding]

@pieh @sidharthachatterjee This should commit the changes. Sorry, what was a simple PR for me, quickly accelerated past my experience level.

Thanks for all you all do.
Chuck.

[pieh]

@pieh @sidharthachatterjee This should commit the changes. Sorry, what was a simple PR for me, quickly accelerated past my experience level.

It wasn't really expected to be so complex from my initial issue discovery. Don't worry, and thanks for being so responsive here!

[pieh]

This should be good to go now