ember-cli / broccoli-asset-rev

Broccoli plugin to add fingerprint checksums and CDN URLs to your assets
MIT License
86 stars 83 forks source link

Enable fingerprinting of the asset map. #74

Closed arjansingh closed 9 years ago

arjansingh commented 9 years ago

First, broccoli-filter@1.2.2 that started breaking this module. I locked the package version to fix that.

Second, like the Rails Manifest, we'd like to be able fingerprint the Asset Map.

I'd like to use the fingerprint unless excluded pattern that manifest.json uses, but that breaks backwards compatibility. Accordingly, I added fingerprintAssetMap as an option. It defaults to false.

I added tests to cover the new use cases.

Can you accept this pull request?

jakiestfu commented 9 years ago

:+1:

rickharrison commented 9 years ago

How does broccoli-filter break the module? All of the tests are still passing.

arjansingh commented 9 years ago

After doing npm cache clean, npm i, and running npm test on master with broccoli-filter@1.2.2 locally:

[prompt: broccoli-asset-rev (prod)] $git branch -vv
  asset-map             47e05fe [origin/master: ahead 1] Fingerprinted asset-map.json files.
  async                 2dc1b8a Use async
  fingerprint-asset-map 4a3e693 [upstream/master: ahead 2] Enable fingerprinting of the asset map.
  lockdown-filter       11021ed [rickharrison/master: ahead 1] Lock down broccoli-filter dependency.
  master                af89d71 [rickharrison/master: ahead 8, behind 24] Merge branch 'upstream-master'
* prod                  dd1e1eb [rickharrison/master] Merge pull request #72 from joliss/annotation
[prompt: broccoli-asset-rev (prod)] $npm ls --depth=0
broccoli-asset-rev@2.1.2 /Users/asingh/code/broccoli-asset-rev
├── broccoli@0.13.6
├── broccoli-asset-rewrite@1.0.9
├── broccoli-filter@1.2.2
├── broccoli-merge-trees@0.2.3
├── json-stable-stringify@1.0.0
├── mocha@2.2.5
├── rsvp@3.0.21
├── sinon@1.16.1
└── walk-sync@0.1.3

[prompt: broccoli-asset-rev (prod)] $npm test

> broccoli-asset-rev@2.1.2 test /Users/asingh/code/broccoli-asset-rev
> mocha tests

  broccoli-asset-rev
    1) revs the assets and rewrites the source
    2) revs the assets when it is not the first plugin
    3) generates a rails-style asset manifest if requested
    4) doesn't fingerprint rails-style manifest if excluded
    5) will prepend if set
    ✓ skips fingerprint and prepends when set and customHash is null
    ✓ skips fingerprint when customHash is null
    6) replaces the correct match for the file extension
    7) uses customHash string value
    8) uses customHash function value
    9) creates a sprockets-style manifest
    10) merges the generated manifest with the sprockets manifest

  2 passing (209ms)
  10 failing

  1) broccoli-asset-rev revs the assets and rewrites the source:
     TypeError: Arguments to path.join must be strings
      at Object.posix.join (path.js:488:13)
      at Fingerprint.getDestFilePath (lib/fingerprint.js:75:24)

  2) broccoli-asset-rev revs the assets when it is not the first plugin:
     TypeError: Arguments to path.join must be strings
      at Object.posix.join (path.js:488:13)
      at Fingerprint.getDestFilePath (lib/fingerprint.js:75:24)

  3) broccoli-asset-rev generates a rails-style asset manifest if requested:
     TypeError: Arguments to path.join must be strings
      at Object.posix.join (path.js:488:13)
      at Fingerprint.getDestFilePath (lib/fingerprint.js:75:24)

  4) broccoli-asset-rev doesn't fingerprint rails-style manifest if excluded:
     TypeError: Arguments to path.join must be strings
      at Object.posix.join (path.js:488:13)
      at Fingerprint.getDestFilePath (lib/fingerprint.js:75:24)

  5) broccoli-asset-rev will prepend if set:
     TypeError: Arguments to path.join must be strings
      at Object.posix.join (path.js:488:13)
      at Fingerprint.getDestFilePath (lib/fingerprint.js:75:24)

  6) broccoli-asset-rev replaces the correct match for the file extension:
     TypeError: Arguments to path.join must be strings
      at Object.posix.join (path.js:488:13)
      at Fingerprint.getDestFilePath (lib/fingerprint.js:75:24)

  7) broccoli-asset-rev uses customHash string value:
     TypeError: Arguments to path.join must be strings
      at Object.posix.join (path.js:488:13)
      at Fingerprint.getDestFilePath (lib/fingerprint.js:75:24)

  8) broccoli-asset-rev uses customHash function value:
     TypeError: Arguments to path.join must be strings
      at Object.posix.join (path.js:488:13)
      at Fingerprint.getDestFilePath (lib/fingerprint.js:75:24)

  9) broccoli-asset-rev creates a sprockets-style manifest:
     TypeError: Arguments to path.join must be strings
      at Object.posix.join (path.js:488:13)
      at Fingerprint.getDestFilePath (lib/fingerprint.js:75:24)

  10) broccoli-asset-rev merges the generated manifest with the sprockets manifest:
     TypeError: Attempted to wrap statSync which is already wrapped
      at Context.<anonymous> (tests/filter-tests.js:233:11)
  --------------
  Error: Stack Trace for original
      at Context.<anonymous> (tests/filter-tests.js:207:11)

npm ERR! Test failed.  See above for more details.
[prompt: broccoli-asset-rev (prod)] $

Edited: for typos

arjansingh commented 9 years ago

Doing the same but locking to broccoli-filter@1.2.0:

[prompt: broccoli-asset-rev (prod)] $git diff
diff --git a/package.json b/package.json
index 89b2cba..678d6ce 100644
--- a/package.json
+++ b/package.json
@@ -31,7 +31,7 @@
   "homepage": "https://github.com/rickharrison/broccoli-asset-rev",
   "dependencies": {
     "broccoli-asset-rewrite": "^1.0.9",
-    "broccoli-filter": "^1.1.0",
+    "broccoli-filter": "1.2.0",
     "json-stable-stringify": "^1.0.0",
     "rsvp": "~3.0.6"
   },
[prompt: broccoli-asset-rev (prod)] $npm ls --depth=0
broccoli-asset-rev@2.1.2 /Users/asingh/code/broccoli-asset-rev
├── broccoli@0.13.6
├── broccoli-asset-rewrite@1.0.9
├── broccoli-filter@1.2.0
├── broccoli-merge-trees@0.2.3
├── json-stable-stringify@1.0.0
├── mocha@2.2.5
├── rsvp@3.0.21
├── sinon@1.16.1
└── walk-sync@0.1.3

[prompt: broccoli-asset-rev (prod)] $npm test

> broccoli-asset-rev@2.1.2 test /Users/asingh/code/broccoli-asset-rev
> mocha tests

  broccoli-asset-rev
    ✓ revs the assets and rewrites the source (82ms)
    ✓ revs the assets when it is not the first plugin (46ms)
    ✓ generates a rails-style asset manifest if requested
    ✓ doesn't fingerprint rails-style manifest if excluded
    ✓ will prepend if set
    ✓ skips fingerprint and prepends when set and customHash is null
    ✓ skips fingerprint when customHash is null
    ✓ replaces the correct match for the file extension
    ✓ uses customHash string value
    ✓ uses customHash function value
    ✓ creates a sprockets-style manifest (41ms)
    ✓ merges the generated manifest with the sprockets manifest (41ms)

  12 passing (382ms)

[prompt: broccoli-asset-rev (prod)] $
pwfisher commented 9 years ago

:+1:

rickharrison commented 9 years ago

@joliss Do you know what is causing this to break?

arjansingh commented 9 years ago

@rickharrison if it's all working on your end I have no objections to omitting https://github.com/dollarshaveclub/broccoli-asset-rev/commit/e6234414aa91da6eca92cb595d72a5b0a844f4bd from my PR.

Added: You could also try clearing your npm cache with npm cache clean in case you've got an old version of the package locally.

joliss commented 9 years ago

@rickharrison I sent a fix for the broccoli-filter breakage in #75.

rickharrison commented 9 years ago

Sorry about that. A fix is published at 2.1.3. Could you rebase/squash and then I'll review?

arjansingh commented 9 years ago

@rickharrison you got it. Coming right up.

arjansingh commented 9 years ago

@rickharrison commit is rebased and ready for review.

arjansingh commented 9 years ago

@rickharrison Are you going to get a chance to review it this week?

rickharrison commented 9 years ago

Sorry, thanks for the contribution!

arjansingh commented 9 years ago

Any chance you can publish 2.2.0? :smile:

rickharrison commented 9 years ago

Apologies. Its published now.

arjansingh commented 9 years ago

:+1: