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

fingerpint support for requirejs #7

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

Conversation

avalez
Copy link
Contributor

@avalez avalez commented Jan 16, 2015

Note, it's not optimal:

  • requires listing files that one wants to "watch" for fingerprint
  • calculates hash twice

Side note, I seem have to use requirejs, because of requirejs/almond/issues/99

@Tug
Copy link
Owner

Tug commented Jan 16, 2015

Well the issue here is that assetGroup.hash() is going to return the hash of the main file only. What we want is the hash of all the files combined.
But it's not really possible with require.js as files are loaded dynamically. So I think we have no choice but take the hash of the compiled output script.

@avalez
Copy link
Contributor Author

avalez commented Jan 16, 2015

Try changing test/public/js/controllers/test.js, both tests fail:

  1. Asset Middleware in production should process JS assets using concatenation and minification in production:
    Error: ENOENT, open '/opt/jira/temp/express-asset-manager/test/builtAssets/js/4057a108527a98e65a6b72ae786a3463-all.js'

  2. Asset Middleware in production should process JS assets with requirejs in production:
    Error: ENOENT, open '/opt/jira/temp/express-asset-manager/test/builtAssets/js/5451665593271599f1f3b9544ec8eaeb-app.js'

Meaning that sum changed!

In requirejs it seems that 'files' array is used by hash() function, but not for anything else. So it's up to developer to make it meaningful.

From other side, I've tried creating hash() on compiled output, but it's too late, data-main attribute is already rendered to html. I've also tried to use furl from static-expiry (it builds cache with hashes in memory), pretty tricky, but still not functional - same problem - compiled output is not yet generated when static-expiry builds cache, upon rendering page. I've thought that making serial execution of AssetGroupProcessors, could solve the problem (it also would be quite convenient for production mode, so app is started in ready state), but appeared overcomplicated to me, opposite to the approach in the pull request.

@avalez
Copy link
Contributor Author

avalez commented Jan 20, 2015

Hi Tug,

Let me kindly bring it up.

@avalez
Copy link
Contributor Author

avalez commented Sep 20, 2015

Hi Tug,

Let me check if you have any plans about it?

With last update today, it does not calculate hash twice, so it's a bit better.

Thank you.

@Tug
Copy link
Owner

Tug commented Sep 20, 2015

Thanks for your contribution @avalez, it's greatly appreciated :)

However I'm not convinced yet of the utility of this patch. The md5 hash is indeed correct because you specified the list of files manually in your config (through the files property) but this is not needed in require.js as files are loaded dynamically by the compiler. I wouldn't want to specify the list of files manually in any large project compiled with require.js.

I really think the way to go is to postprocess the output, just take the hash of the output file itself and modify its name by appending the hash of its content. Don't you think ?

@avalez
Copy link
Contributor Author

avalez commented Sep 20, 2015

Ok, I agree, my approach is not correct. Doing postprocess seems right to me. But can you implement it please? I've not managed so far :)

In any case this pull request might be closed.

@Tug
Copy link
Owner

Tug commented Sep 20, 2015

Yeah it's not so easy because there are many cases to handle. For instance in require.js without the includeLib option, the main file is the require.js library, the generated app.js file is just a dependency but it is the one we want to add the hash to. Then we need to update properties filepath, url, assets, attributes... I feel that it needs quite a bit of refactoring not to make the code too ugly.
I see if I have some time to improve on this ;)

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

Successfully merging this pull request may close these issues.

2 participants