Skip to content
This repository has been archived by the owner on Jul 24, 2022. It is now read-only.

refactoring & improvements #43

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

BusinessDuck
Copy link

@BusinessDuck BusinessDuck commented Dec 15, 2018

PR features

  • Performance issues

    • Too many buffers conversions
    • For loop overhead in comparisons and replace operations
    • Prevent object mutations in loops
  • External plugins completable increased

  • Others bugs and issues

    • Solved relative paths problem
    • Solved unreplaced img refs in css files, when hexo images is behind of css files
    • Code style improved
    • ReadMe added new features
  • New features

    • Keep original files without replacing assets refs to revisioning files
    • Now you can replace assets refs in ignored files, but not filename of itself by replace_in_ignored opts
    • Relative paths is supported now and may be customized by relative_dirs opts
    • Imagemin support excluded opts. For example you need to optimize only css/images/** files

@tomap
Copy link
Contributor

tomap commented Mar 15, 2019

Hi,

I have a few feedback on your PR:

1/ Thank you

2/
I see that you made multiple independent changes.
To ease the review of those changes, could you isolate them in separate PR?
Also, you pushed the package-lock.json
Although that is a good practice in general, please don't do it in this PR

Also, you have not pushed any unit test.

Maybe someone familiar with this plugin will want to review your PR "as is"? @hexojs/core ?

@BusinessDuck
Copy link
Author

Already use in production that during past six months. Have no issues yet.
Too many changes, and too many time is gone sorry can't make a separate pr's.
Would be great if someone can review from hexo core team.
About units, sure i can cover new features and remove lock json as well.

@bhaskarmelkani
Copy link
Member

I will do a review ASAP.

@bhaskarmelkani
Copy link
Member

Hey,

Can you remove package-lock.json and add some unit tests, it will make PR easy to read and understand.

I highly appreciate your contribution.

@BusinessDuck
Copy link
Author

BusinessDuck commented Mar 27, 2019

Added package-lock fix. And i tried to add some units, but i got strange errors in sandbox "ReferenceError: hexo is not defined" when process(ctx) is called (check my todos in css tests)

@bhaskarmelkani
Copy link
Member

Hey @BusinessDuck,

I replaced hexo-asset-pipeline with
BusinessDuck:master in my setup(that was working fine before) and got following error.

TypeError: Cannot read property 'length' of undefined
    at checkFileIgnore (/Users/bmelkani/Projects/github/hexojs/tmp/hexo-asset-pipeline/lib/utils.js:43:36)
    at /Users/bmelkani/Projects/github/hexojs/tmp/hexo-asset-pipeline/lib/filters/image.js:58:12
    at Array.filter (<anonymous>)
    at Hexo.run (/Users/bmelkani/Projects/github/hexojs/tmp/hexo-asset-pipeline/lib/filters/image.js:55:31)
...

To me changes in your PR looks, like breaking changes (which might also have some bugs or miss some corner cases). This will break working setups for users.

Since the change suggested by you is mostly code refactoring, sorry I am afraid that I would not be able to merge these changes.

I hope you understand this. Thanks for your time. If you can provide some small PRs then we can incrementally incorporate them in the plugin.

Thanks :)

@BusinessDuck
Copy link
Author

no problem, i'll try to make separate pr's later. Whats about my question regarding with hexo sandbox?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants