broccoli-asset-rev icon indicating copy to clipboard operation
broccoli-asset-rev copied to clipboard

Switch to using broccoli-persistent-filter

Open elucid opened this issue 9 years ago • 6 comments

See #92

This updates broccoli-asset-rev to use broccoli-persistent-filter. Although the APIs are similar, it was not possible to do a simple swap-out replacement. In particular, the previous implementation relied on getDestFilePath() not being called for certain relativePaths for which it is now being called. While this could have been worked around, we opted to refactor a bit and use more of broccoli-persistent-filter's public API. The hashing is now done within processString() which is passed in each file's contents so we now avoid an extra readFileSync for every file.

I should explain all of the text fixture renames: hashing the contents parameter of processString() results in different digests for the binary files in the test fixture directories (previously we were hashing a Buffer and now a string).

elucid avatar Apr 01 '16 21:04 elucid

Current coverage is 93.37%

Merging #93 into master will decrease coverage by -2.02% as of 3f51d43

@@            master     #93   diff @@
======================================
  Files            3       3       
  Stmts          152     151     -1
  Branches        36      38     +2
  Methods          0       0       
======================================
- Hit            145     141     -4
  Partial          2       2       
- Missed           5       8     +3

Review entire Coverage Diff as of 3f51d43

Powered by Codecov. Updated on successful CI builds.

codecov-io avatar Apr 01 '16 21:04 codecov-io

@stefanpenner Would you mind reviewing and signing off?

rickharrison avatar Apr 02 '16 22:04 rickharrison

Will do

stefanpenner avatar Apr 03 '16 18:04 stefanpenner

Just tried this and the new broccoli-asset-rewrite PR, unfortunately they don't appear to work correctly.

When used together, it appears broccoli-asset-rewrite seems to prevent assets on disk from getting the correct fingerprinted name, removing it and all appears well.

The issue might be in the other project, but requires a deeper look.

stefanpenner avatar Apr 04 '16 19:04 stefanpenner

Is this PR dead? I want to add pluggable getDestFilePath but it would obviously clash with the change to processString(), so I'd like to know if this is happening.

qm3ster avatar May 19 '16 16:05 qm3ster

@stefanpenner @elucid @rickharrison I too wonder about the status of this PR/repository. This particular PR is 14 months old now. How's the support?

melv-n avatar Jul 11 '17 11:07 melv-n