express-asset-manager icon indicating copy to clipboard operation
express-asset-manager copied to clipboard

fingerpint support for requirejs

Open avalez opened this issue 10 years ago • 7 comments

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 jrburke/almond/issues/99

avalez avatar Jan 16 '15 17:01 avalez

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.

Tug avatar Jan 16 '15 17:01 Tug

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 avatar Jan 16 '15 18:01 avalez

Hi Tug,

Let me kindly bring it up.

avalez avatar Jan 20 '15 12:01 avalez

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.

avalez avatar Sep 20 '15 17:09 avalez

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 ?

Tug avatar Sep 20 '15 17:09 Tug

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.

avalez avatar Sep 20 '15 18:09 avalez

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

Tug avatar Sep 20 '15 22:09 Tug