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] refactor/plugin system #43

Closed
wants to merge 2 commits into from
Closed

Conversation

efegurkan
Copy link
Collaborator

Rewrite and rearrange utilities into plugins.

-Create sqip.js to run plugins over the input file
-Turn SVG, SVGO, primitive and base64 as plugins
-Move loadSVG into utils/helpers

@efegurkan
Copy link
Collaborator Author

I started experimenting with a possible implementation of the plugin system for a while ago. I will reintegrate after #42 has been merged.

@axe312ger Feedback would be awesome since you have better experience with node compared to me :) Also I am pretty sure you have something in your mind as well.

Copy link
Owner

@axe312ger axe312ger left a comment

Choose a reason for hiding this comment

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

Added some comments. This goes very closely to what I have in mind! Thanks for helping here :)

@@ -0,0 +1,29 @@
const encodeBase64 = rawSVG => Buffer.from(rawSVG).toString('base64')
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if we should remove this completely. Base64 is just bad for compression. See data here:

gatsbyjs/gatsby#4205 (comment)

Solution: https://www.npmjs.com/package/mini-svg-data-uri

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@technopagan had something about that I can agree on that <3 also the wrapping we do, not sure if it is useful at all.

Copy link
Owner

Choose a reason for hiding this comment

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

Actually I expected the CLI to spit out a SVG file, not an Image tag 🙈

We could keep base64 as plugin and use the mini data uri's as default for encoding.

src/plugins/svgo.js Outdated Show resolved Hide resolved
src/sqip.js Outdated
const SVGPlugin = require('./plugins/svg')
const SVGOPlugin = require('./plugins/svgo')
const PrimitivePlugin = require('./plugins/primitive')
const Base64EncodePlugin = require('./plugins/base64')
Copy link
Owner

Choose a reason for hiding this comment

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

i wonder if we should at least export them as separate packages with something like https://github.com/lerna/lerna

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, we are actually using it for melody at trivago.

@axe312ger axe312ger mentioned this pull request Jul 17, 2018
7 tasks
@efegurkan
Copy link
Collaborator Author

@axe312ger @technopagan Hey I am sorry I cannot find any more time to continue on it currently. A lot has been happening since last 6 months (Personal stuff, work change etc.). I feel like I am blocking everything. Please feel free to take over this, or build a new one. I will still try to do contribute later, but cannot find free time anymore.

Sorry 🤷‍♂️

@axe312ger axe312ger changed the base branch from next to master March 2, 2019 07:52
@axe312ger
Copy link
Owner

@hafizalfaza this is something you could have a look at. Rebase, fix conflicts and then check how much of @efegurkan code we can use. I guess it is in a pretty good state.

Rewrite and rearrange utilities into plugins.

-Create sqip.js to run plugins over the input file
-Turn SVG, SVGO, primitive and base64 as plugins
-Move loadSVG into utils/helpers
@axe312ger axe312ger force-pushed the refactor/plugin-system branch from 023be72 to 8bd23d3 Compare April 8, 2019 10:06
@codecov-io
Copy link

codecov-io commented Apr 8, 2019

Codecov Report

Merging #43 into master will decrease coverage by 6.1%.
The diff coverage is 90.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
- Coverage   90.72%   84.61%   -6.11%     
==========================================
  Files           5        7       +2     
  Lines          97      117      +20     
  Branches       11       14       +3     
==========================================
+ Hits           88       99      +11     
- Misses          8       15       +7     
- Partials        1        3       +2
Impacted Files Coverage Δ
src/plugins/primitive.js 100% <100%> (ø)
src/plugins/svgo.js 100% <100%> (ø)
src/utils/helpers.js 100% <100%> (ø) ⬆️
src/plugins/base64.js 100% <100%> (ø)
src/index.js 80.76% <66.66%> (-19.24%) ⬇️
src/plugins/svg.js 90.24% <90.24%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 528ec10...a411ef8. Read the comment docs.

@axe312ger axe312ger mentioned this pull request Apr 22, 2019
@axe312ger
Copy link
Owner

code went into core via #55

@axe312ger axe312ger closed this May 11, 2019
@axe312ger axe312ger deleted the refactor/plugin-system branch April 23, 2021 11:48
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