split fingerprint + rewrite into two stages to address #118

[cbrevdev]

• first stage excludes replaceable source code from fingerprinting
• second stage fingerprints replaceable source code files allowing fingerprint changes in non-source code assets to be reflected as fingerprint changes in source code assets
• update source code tests assets to have fingerprint match file hash
• extended asset map test to confirm generation is still correct

[rickharrison]

Full disclosure: I am not going to have time to review this in the near future. If someone from the ember team could review, test, and sign off, it would be much appreciated.

[cbrevdev]

Understood. I've created an upstream issue to try to gather some interest in reviewing this PR.

To be clear this PR is specific to addressing the case where assets are referenced by source code assets. This side steps the problem of having to consider circular dependencies, at the cost of continuing to miscalculate the fingerprints of source code assets when they import other source code assets.

At the least, it seems like this is the main issue being encountered. As such I'm unsure whether it is worth the effort/complexity of complete asset dependency graph calculation. Especially as the rewrite logic is handled by a separate module (broccoli-asset-rewrite)...

[makepanic]

I found a potentially breaking change depending on how the behavior was defined.

I previously added emojis inside the exclude array. After using the PR dependency, i had to update all exclude values to glob match.

This means something like:

exclude: ['img/leaflet', 'emojis']

which excluded /dist/assets/img/leaflet/** and /dist/assets/emojis/**

had to be rewritten to:

exclude: ['**/img/leaflet/**', '**/emojis/**']

I'm not sure if it was correct previously because https://ember-cli.com/user-guide/#fingerprinting-and-cdn-urls says something like ['fonts/169929'] should work

[cbrevdev]

Good catch. Similar to the possible .js/.json confusion, I was concerned that "emojis" would not only exclude "emojis/", but also files such as "emojis.js", "more-emojis.html", etc.

As part of commit a9484bf; non-glob exclusions are treated as prefixes. You're right that this is a breaking change. I'll revert back to the previous behavior.

That said, I would recommend preferring the glob syntax, as it avoids potentially surprising exclusions.

[mirague]

Good catch! Perhaps the change to glob syntax could be a PR of its own for the next major release?

[cbrevdev]

Separate PR is a great idea. Either a breaking change to just globs, or perhaps we update the docs to mention the string search behavior along with its caveats?

For now I'm going to hold off on additional PR's until I can figure out how to get traction with this one.

[mirague]

I wonder who to approach from the Ember team. Perhaps we can ask in #general on the Ember Slack channel.

Edit: Posted in #general

[mirague]

I'm unsure how to get traction for this PR. broccoli-asset-rev hasn't been updated for 10 months. Not sure who to approach directly as @rickharrison does not have time for this repo at this point in time.

cc @arjansingh @rwjblue @joliss

[cbrevdev]

css @import will still be handled incorrectly by this PR. We'd indeed need to change to an dependency graph scheme (mentioned by #29), such as asset-graph. I'd recommend doing this as a separate PR or perhaps a separate Broccoli module?

[rwjblue]

Thank you @cbrevdev!

[rwjblue]

Released as v2.6.0

[rickharrison]

Thanks @rwjblue !!!

[rickharrison]

My app is no longer working. Are we sure this did not introduce a regression?

[cbrevdev]

If your app stopped working, then sounds like there could be a regression. While we've moved over to v2.6.0 without issue, and unit tests are passing. It's certainly possible I missed something.

Are you able to provide further details including the fingerprint config?

[k3n]

@rickharrison any luck diagnosing the issue?

[mirague]

@cbrevdev Do you think this approach could be applied for a similar issue such as https://github.com/kimroen/ember-cli-autoprefixer/issues/38?

[cbrevdev]

Thanks for the heads up!

Added comments + PR over there :)