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

CLI: update sharp (for node 12 compatibility) #1471

Merged
merged 1 commit into from
Apr 30, 2019

Conversation

chrmoritz
Copy link
Contributor

This PR is part 1 / 2 of what is needed to make joplin cli compatible with node 12.
Part 2 will be to update sqlite3 to a version with node 12 support. Unfortunately the fix for it didn't make it into a stable version yet (see also: TryGhost/node-sqlite3#1138), so we still have to wait for that.

This is currently the last remaining blocker for Homebrew to make all node formulae compatible with the latest current node 12 release in Homebrew/homebrew-core#39188.

In this PR the version of the joplin cli dependency sharp is bumped from 0.20.8 to 0.22.1 for node compatibility and some deprecated and now removed function calls are replaced with their suggested replacements.

@tessus
Copy link
Collaborator

tessus commented Apr 29, 2019

If you update sharp will it still run on node 8 and node 10?

@chrmoritz
Copy link
Contributor Author

chrmoritz commented Apr 29, 2019

If you update sharp will it still run on node 8 and node 10?

Yes, it will still run on node 6+ and it's tested on node 6/8/10/11/12 (see sharp's travis and appveyor config).

BTW: The for node 12 support required commit is lovell/sharp@aa9b328.

@laurent22
Copy link
Owner

Sharp was downgraded at some point to fix this bug: #102

If we upgrade it now, will we have this bug again or will it be fixed?

@chrmoritz
Copy link
Contributor Author

chrmoritz commented Apr 29, 2019

@laurent22 I've only tested the CLI version of joplin with this PR yet and I can confirm that image resizing is working fine there.

But if I'm reading the resizeImage_ code correctly sharp isn't used at all for the desktop version (Edit: since d4b19f1), but electron's nativeImage is used there instead. Maybe sharp was used in some older version for the electron client too (Edit: it was) and caused some issues leading to the downgrade from 0.18 to 0.17 in the past. But since then sharp was already upgraded to version 0.20.8 (=current used version).

Because of that I don't think that #102 is still relevant. This change should only affect the CLI version and not the electron desktop version.

@laurent22
Copy link
Owner

Yes you're correct, I got confused between the CLI and Electron app.

So I guess this PR is fine then. Did you test if adding a large image to a note will downsize it as expected?

@chrmoritz
Copy link
Contributor Author

chrmoritz commented Apr 29, 2019

Did you test if adding a large image to a note will downsize it as expected?

Yes, it got downsized to the 1920px max (also the aspect ratio is preserved and it isn't enlarge if you attach a smaller image).

@laurent22
Copy link
Owner

Perfect, thanks @chrmoritz, I think we can merge this as a first step then.

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