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

buildVersionHash is not doing what is suppose to do #31

Open
dariomac-vix opened this issue Apr 9, 2019 · 8 comments
Open

buildVersionHash is not doing what is suppose to do #31

dariomac-vix opened this issue Apr 9, 2019 · 8 comments

Comments

@dariomac-vix
Copy link

Hi, buildVersionHash has a comment (and historically do this):

Walks the directory tree, finding files, generating a version hash

But from v.3.0.1, the code responsible for the hash creation (cachedMakeHash) is called from getVersionedPath. So, the comment is outdated but most important, the whole concept of

When you initialize this module, it crawls your public folder synchronously at startup, and pre-determines all the MD5 hashes for your static files. This slows down application startup slightly, but it keeps the runtime performance at its peak.

Now, the hash is generated only if the resource is called and make slow the first call of it.

I think that, or you change your code to match the documentation (and how historically works) or you change the description/comment.

Let me know what you think.

@XhmikosR
Copy link
Collaborator

XhmikosR commented Apr 9, 2019

You mean v3.1.0? You can always submit a PR :)

@dariomac-vix
Copy link
Author

Yes... sorry, v3.1.0... and yes... I can submit a PR.

I'm only asking because it was a big change and maybe it has some reason behind that I'm not aware. I'll test some changes locally and submit a PR asap.

@XhmikosR
Copy link
Collaborator

XhmikosR commented Apr 9, 2019

It was just a leftover I believe. Because the buildVersionHash does do what it says, but now it depends on cachedMakeHash so the comment(s) might need to be adapted..

@dariomac-vix
Copy link
Author

dariomac-vix commented Apr 9, 2019

Mmm but looking at the code I can't figure it out how it does what it says, because in any case call cachedMakeHash and that's is the only place that called crypto.createHash

@XhmikosR
Copy link
Collaborator

XhmikosR commented Apr 9, 2019

@Rush: do you have any comments since you were the author of the patch?

@Rush
Copy link
Contributor

Rush commented Apr 9, 2019

Comment is no longer valid. We shouldn't by default warm up the cache. (Perhaps optionally) Why would we slowdown the server upon every reload? Think of nodemon or node-dev in development.

@Rush
Copy link
Contributor

Rush commented Apr 9, 2019

See my previous PR for context #23

@dariomac-vix
Copy link
Author

I think it can be optional as you say... With your context message I understand why you did that. Also, not only the comment is no longer valid, but parts of the readme.md too (as the one I quoted early).

Tonight I'll update my PR trying to include changes in the readme.md

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

No branches or pull requests

3 participants