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

Switch to using broccoli-persistent-filter #93

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

Conversation

elucid
Copy link
Contributor

@elucid elucid commented Apr 1, 2016

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

@codecov-io
Copy link

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.

@@ -24,10 +24,15 @@ function Fingerprint(inputNode, options) {

options = options || {};

if (typeof options.persist === 'undefined') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

options.persist === undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

NOTE:
* overrides fewer `Filter` functions
* simplifies implementation by making use of public APIs
* avoids an extra readFileSync for every resource because hashing
  is done within `processString` (which has contents passed in as a
  param) rather than in `getDestFilePath`
* unfortunately all image files (and files which reference them) in
  text fixtures had to be renamed because now the file contents as
  string rather than a Buffer are passed to the hash function and
  consequently produce different digests
@rickharrison
Copy link
Collaborator

@stefanpenner Would you mind reviewing and signing off?

@stefanpenner
Copy link
Collaborator

Will do

@stefanpenner
Copy link
Collaborator

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.

@qm3ster
Copy link

qm3ster commented May 19, 2016

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.

@mirague
Copy link

mirague commented Jul 11, 2017

@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?

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

Successfully merging this pull request may close these issues.

6 participants