hexo-asset-pipeline icon indicating copy to clipboard operation
hexo-asset-pipeline copied to clipboard

refactoring & improvements

Open BusinessDuck opened this issue 6 years ago • 7 comments

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

BusinessDuck avatar Dec 15 '18 12:12 BusinessDuck

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 ?

tomap avatar Mar 15 '19 21:03 tomap

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.

BusinessDuck avatar Mar 16 '19 13:03 BusinessDuck

I will do a review ASAP.

bhaskarmelkani avatar Mar 18 '19 20:03 bhaskarmelkani

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.

bhaskarmelkani avatar Mar 20 '19 20:03 bhaskarmelkani

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)

BusinessDuck avatar Mar 27 '19 10:03 BusinessDuck

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 :)

bhaskarmelkani avatar Mar 28 '19 16:03 bhaskarmelkani

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

BusinessDuck avatar Mar 29 '19 16:03 BusinessDuck