philipp-winterle / crittr

High performance critical css extraction with a great configuration abilities
https://hummal.github.io/crittr/
GNU General Public License v3.0
51 stars 6 forks source link

feat: adds option `screenshotNameGenerator` to customize screenshots' name #38

Closed rmachado-studocu closed 2 years ago

rmachado-studocu commented 2 years ago

What does this do:

Why do we need this?

While using this library I've encountered situations where the URL was too long and caused E_NAME_TOO_LONG errors when generating the screenshots. Having a function to allow us to shrink the name (as in the added test, using crypto to generate a hash of the URL for example) allows us to pass the control to the user of the library.

philipp-winterle commented 2 years ago

Thank you for your PR and your effort!

Not sure the function to prevent E_NAME_TOO_LONGis the best idea. You will still encounter this situation and then you can use the function to prevent it. Shouldn't it just not happen?

I mean the best way would be to cut too long filenames. Or do the naming anyway to prevent the error at all in every situation for everyone.

Like: DOMAIN_DATETIME.png or something like that

rmachado-studocu commented 2 years ago

Hi @hummal !

Well, it can happen indeed in both situations, but the function gives the control to the users to determine what logic they want to use to name their files.

DOMAIN_DATETIME.png would be one way, but then it's unpredictable - let's say I want to copy (or verify) if the screenshot is created - I need to check in the filesystem for the existing names. I also would need to know which was generated first.

Another approach could be using a static name combined with the index of the url: SCREENSHOT-URL-0.png

Another reason why I didn't change the screenName's initial implementation was due to the possibility of having already users based on the previous logic and that would make it a breaking change. 😉

Let me know your thoughts. If you still want me to change it to DOMAIN_DATETIME.png I can also do it, no problem.

Thanks for your time making this project, reviewing my PR and providing feedback! 👍

philipp-winterle commented 2 years ago

DOMAIN_DATETIME.png would be one way, but then it's unpredictable - let's say I want to copy (or verify) if the screenshot is created - I need to check in the filesystem for the existing names. I also would need to know which was generated first.

Another approach could be using a static name combined with the index of the url: SCREENSHOT-URL-0.png

Another reason why I didn't change the screenName's initial implementation was due to the possibility of having already users based on the previous logic and that would make it a breaking change. 😉

Let me know your thoughts. If you still want me to change it to DOMAIN_DATETIME.png I can also do it, no problem.

Thanks for your time making this project, reviewing my PR and providing feedback! 👍

Breaking is a good point. We will leave it this way for now. If there is a need I hope they will create some issues.

rmachado-studocu commented 2 years ago

Thanks @hummal ! :) Have a nice day!