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: copy static image as it is if image compression fail #887

Merged
merged 2 commits into from
Aug 4, 2018

Conversation

endiliey
Copy link
Contributor

@endiliey endiliey commented Aug 4, 2018

Motivation

Fix #701

There is no error handling on https://github.com/facebook/Docusaurus/blob/ef80581e8ef950a0865fc7851407c672c4bbd510/lib/server/generate.js#L255-L264

If the image compression fails, it will cause the image to be missing.

Example:
https://circleci.com/gh/facebook/Docusaurus/2846
error

The build succeed but along the line there is an unhandled promise. Causing christopher-chedeau.jpg and ricky-vetter.jpg to be missing.

missing image

This PR add a fallback functionality to copy the image as it is if the image compression fail

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

https://deploy-preview-887--docusaurus-preview.netlify.com/

Related PRs

#654

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Aug 4, 2018
@endiliey endiliey changed the title fix: copy static image as it is if image compression failed fix: copy static image as it is if image compression fail Aug 4, 2018
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Aug 4, 2018

Deploy preview for docusaurus-preview ready!

Built with commit dfc0619

https://deploy-preview-887--docusaurus-preview.netlify.com

Copy link
Contributor

@JoelMarcey JoelMarcey left a comment

Choose a reason for hiding this comment

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

👍

It would be interesting to see what errors are being thrown at random times to cause images not to compress correctly.

On that note, do you think having the image compression functionality is still useful for us? Or is it causing more problems than it helps?

@@ -247,11 +247,9 @@ async function execute() {
!commander.skipImageCompression
) {
const parts = normalizedFile.split(`${sep}static${sep}`);
const targetDirectory = join(
buildDir,
parts[1].substring(0, parts[1].lastIndexOf(sep))
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this always wrong? I see you removed this from your update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not really wrong but this is not declarative enough. I think using function from nodejs path clearly shows the intent.

const targetDirectory = path.dirname(targetFile);

The previous code in the below section

mkdirp.sync(path.dirname(targetDirectory));

is wrong though. It should be

mkdirp.sync(targetDirectory);

@endiliey
Copy link
Contributor Author

endiliey commented Aug 4, 2018

@JoelMarcey i think it definitely help in the sense that it reduces the website loading page since image size is reduced. It's causing problem because we completely skip that image if compression is failing. Ex: #701

@endiliey endiliey merged commit 5fae14a into facebook:master Aug 4, 2018
@endiliey endiliey deleted the imagemin branch August 4, 2018 14:40
@endiliey
Copy link
Contributor Author

endiliey commented Aug 4, 2018

It still fails but at least the image is now intact and image is copied to build.

Here is the error printed:

{ Error: Error in file: /home/circleci/docusaurus/website/static/img/christopher-chedeau.jpg

spawn /home/circleci/docusaurus/node_modules/jpegtran-bin/vendor/jpegtran ENOENT
    at _errnoException (util.js:992:11)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:190:19)
    at onErrorNT (internal/child_process.js:372:16)
    at _combinedTickCallback (internal/process/next_tick.js:138:11)
    at process._tickCallback (internal/process/next_tick.js:180:9)
  code: 'ENOENT',
  errno: 'ENOENT',
  syscall: 'spawn /home/circleci/docusaurus/node_modules/jpegtran-bin/vendor/jpegtran',
  path: '/home/circleci/docusaurus/node_modules/jpegtran-bin/vendor/jpegtran',
  spawnargs: 
   [ '-copy',
     'none',
     '-optimize',
     '-outfile',
     '/tmp/c0b1b392-9983-40e2-bc2f-dca2418d460c',
     '/tmp/aa77e0b6-cbfa-4f21-b08f-02ed65dff725' ],
  killed: false,
  stdout: '',
  stderr: '',
  failed: true,
  signal: null,
  cmd: '/home/circleci/docusaurus/node_modules/jpegtran-bin/vendor/jpegtran -copy none -optimize -outfile /tmp/c0b1b392-9983-40e2-bc2f-dca2418d460c /tmp/aa77e0b6-cbfa-4f21-b08f-02ed65dff725',
  timedOut: false }
{ Error: Error in file: /home/circleci/docusaurus/website/static/img/ricky-vetter.jpg

@JoelMarcey
Copy link
Contributor

Bad image? Or buggy compressor?

Sent with GitHawk

@ahmadalfy
Copy link
Contributor

@JoelMarcey I think it should copy in either cases ... I didn't notice that case would happen to be honest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants