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

WIP - Image Optimization #1

Open
wants to merge 4 commits into
base: rysolv
Choose a base branch
from

Conversation

ZeeshanZulfiqarAli
Copy link

@ZeeshanZulfiqarAli ZeeshanZulfiqarAli commented Apr 30, 2022

Added pre-processing functionality which can optimize jpg & png images, which consists of two parts:

  1. Creating a webp version of all the images.
  2. Creating a compressed version of the images in the same format as the source.

The HTML produced for the image looks like:

<picture>
    <source srcset="./assets/no-idea-dog.webp" type="image/webp">
    <img src="./assets/no-idea-dog.png" alt="Markdown to HTMl" title="testing test">
</picture>

This helps make sure that if the client does not support webp, it can fall back to the source format safely.

The image also supports title which can be specified in markdown file as a second parameter, following the href of image:

![Markdown to HTMl](./assets/no-idea-dog.png "This is a title")

Changes to package.json

The pre-built command now executes pre-process.js script which optimize images of specific formats and copies rest of the files as is to the target directory of build/assets. This script supports optional commands:

  • dontCompressImages: add this flag to prevent any optimizations for images. This also prevents webp image generation.
  • dontGenerateWebp: add this flag to prevent webp generation of images in assets.
    Example usage:
    npm run pre-build -- --dontCompressImages
    npm run pre-build -- --dontGenerateWebp

TODO:

  • Add support for more image formats.
  • Configure script arguments to be more user friendly.
  • Provide the ability to turn off image compression only and keep webp generation active.
  • Support script arguments with build command as well.

@netlify
Copy link

netlify bot commented Apr 30, 2022

Deploy Preview for compassionate-saha-53a9e6 ready!

Name Link
🔨 Latest commit d3dc883
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-saha-53a9e6/deploys/626ebbd1ed55da0008f291f2
😎 Deploy Preview https://deploy-preview-1--compassionate-saha-53a9e6.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.


execSync('npm run clean');
execSync('npm run pre-build -- ' + arg, {stdio:[0, 1, 2]});
execSync('node index.js ' + arg, {stdio:[0, 1, 2]});
Copy link
Member

Choose a reason for hiding this comment

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

I think we could do this in Bash, rather than in javascript.

Copy link
Author

Choose a reason for hiding this comment

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

This was done solely because with npm commands there was no apparent way to pass arguments to the pre-build script if it was done this way: npm run clean && npm run pre-build && node index.js

This exact case was discussed here in much detail and this was the solution recommended in the end: https://stackoverflow.com/a/51401577/8816118

"imagemin-webp": "^6.0.0",
"marked": "^4.0.12",
"npm": "^8.8.0",
"yargs": "^17.4.1"
Copy link
Member

Choose a reason for hiding this comment

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

What have you done to my beautiful 1 dependency package.json 😢

Copy link
Member

Choose a reason for hiding this comment

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

  1. Do we need npm as a dependency?
  2. I think we can use a shell script instead of the build.js and avoid yargs
  3. I think the i package came from a typo. Example: npm i i yargs
  4. fs-extra isn't necessary for just the copy function

Copy link
Author

Choose a reason for hiding this comment

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

What have you done to my beautiful 1 dependency package.json 😢

image

Copy link
Author

Choose a reason for hiding this comment

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

  1. Do we need npm as a dependency?
  2. I think we can use a shell script instead of the build.js and avoid yargs
  3. I think the i package came from a typo. Example: npm i i yargs
  4. fs-extra isn't necessary for just the copy function

Yes i think a few of these are typos. With fs-extra, i was feeling lazy and just thought it was easier to install a new package instead of write a for loop 🤷

Will fix these

@tylermaran
Copy link
Member

I'll pull this down later and try using a shellscript instead of the build.js (eliminates some packages).
Also instead of command line arguments, we can make a config.json file where you can update some settings.

{
    author: "Zeeshan",
    authorImage: "../assets/image",
    preprocessImages: true,
    preprocessAuthorImage: true,
}

Removed fs-extra in favor of a util function
@tylermaran
Copy link
Member

tylermaran commented May 3, 2022

Made some updates in the rysolv branch over the last few weeks (https://github.com/rysolv/markdown_ssg/tree/rysolv. T'll try to get those into master so we can merge with this branch.

I've added a few things already (share links, config file, publish (t/f) flag) that should be pretty easy to work in without an issue.

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.

None yet

2 participants