Maintain correctness when rewriting fastboot-capable apps

[ef4]

Apps that are built with fastboot support contain a fastboot.manifest in their package.json. The manifest includes the names of JS and HTML files. These files may be renamed by broccoli-asset-rev.

Up until now, ember-cli-fastboot has been doing hacks to handle adjusting those references itself. But this requires it to have too much knowledge of broccoli-asset-rev, and it forces ember-cli-fastboot to run after broccoli-asset-rev, which is problematic for use-cases like https://github.com/ef4/prember/issues/2.

This PR reverses that approach by considering fastboot.manifest a standard part of the built ember app that broccoli-asset-rev should know how to update for consistency when renaming files.

This PR includes a flag on the addon itself that we can use to migrate ember-cli-fastboot away from its hacky behavior.

[ef4]

Tests are failing on node 0.12. Current ember-cli supports down to 4. I can also update the CI here to cover a more modern set of nodes if that's acceptable.

[rwjblue]

Hmm. Doesn't this library already support rewriting these files via the extensions option? Note, I haven't tried but I would have assumed that adding json to the extensions list would work without changes here. Am I missing some other issue/caveat?

[ef4]

Yes, two caveats. One is that json is broader than you really want, although that would maybe be easily fixable with a new option for whole file patterns rather than extensions.

But the second caveat is that normal rewriting will also apply the prefix (like a CDN url) to the filenames in the fastboot manifest. We want their local names rewritten but we don’t want prefixing, in just this one place.

On Fri, Nov 3, 2017 at 9:20 AM Robert Jackson notifications@github.com wrote:

Hmm. Doesn't this library already support rewriting these files via the extensions option https://github.com/rickharrison/broccoli-asset-rev#options? Note, I haven't tried but I would have assumed that adding json to the extensions list would work without changes here. Am I missing some other issue/caveat?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rickharrison/broccoli-asset-rev/pull/125#issuecomment-341701152, or mute the thread https://github.com/notifications/unsubscribe-auth/AATfMg9qsbC3Ahi7Ud_fNJd8Bgzt9nXpks5syxMrgaJpZM4QQpC7 .

[rwjblue]

Gotcha, thank you.

We want their local names rewritten but we don’t want prefixing, in just this one place.

👍 I think this is the key bit that I was missing.

json is broader than you really want, although that would maybe be easily fixable with a new option for whole file patterns rather than extensions

I would generally prefer this as I think it would provide a bit more flexibility (and remove some of the fastboot coupling that seems a tad out of place here to me).

[rwjblue]

I'm working through what that option might look like. I was thinking something akin to the way eslint allows overrides, what do you think?

var assetNode = new AssetRev(node, {
  extensions: ['js', 'css', 'png', 'jpg', 'gif'],
  exclude: ['fonts/169929'],
  replaceExtensions: ['html', 'js', 'css'],
  prepend: 'https://subdomain.cloudfront.net/',
  overrides: [
    {
      files: 'fastboot-manifest.json',
      prepend: null
    },
 ]
});

[ef4]

This seems OK but it creates the need for an additional API.

When somebody installs ember-cli-fastboot, I don't want to need to tell them to manually paste some new options into their ember-cli-build.js. We need ember-cli-fastboot to provide the configuration directly to broccoli-asset-rev.

AFAIK there are only hacky ways to do that. Which is a larger topic.

[rwjblue]

I guess I assumed that ember-cli-fastboot would configure the fingerprint options on their behalf.

included() {
  let fingerprintOptions = this.app.options.fingerprint;
  let overrides = fingerprintOptions.overrides = fingerprintOptions.overrides || [];
  overrides.push({
      files: 'fastboot-manifest.json',
      prepend: null
  })
}

🤔

[ef4]

Right, but I consider this hacky for a few reasons.

One is that it's order dependent: we now need to force ember-cli-fastboot's included hook to run before broccoli-asset-rev's (because broccoli-aset-rev is free to copy off its options and start using them right away). While that's not terrible in this particular case (my original problem was that ember-fli-fastboot was running after asset-rev), it's still not ideal. It should be possible for an addon that needs to run after asset-rev to also change settings in asset-rev.

Second is that we're reaching in from far away and mutating state. Which makes things hard to reason about. "Where the heck did this setting come from? I don't know LOL, try grepping."

Third is that we're putting the policy decision of how to merge configs from the app and the addons into the hands of every individual addon. Once you consider that a third addon may want to also alter the fingerprint settings, the combinations get bad.

I realize we only have compromised choices here. Though this discussion is feeding my notes file on how to redesign all addon API.

[ef4]

I would like to argue for merging this PR, based on reiterating my original point:

Fastboot is a first-class feature in Ember. It is not some weird third-party thing. The fact that fastboot-enabled apps include a manifest that makes them runnable by the fastboot library is just something that any addon like this one needs to understand if it wants to be able to do rewriting while maintaining correctness.

If we feel bad about addons needing to understand the manifest, then we should design away the manifest. But right now it is fundamental to the architecture. It is strictly less bad than the opposite alternative, which is attempting to make fastboot understand all possible addons in order to configure them correctly.

[ef4]

This is blocking my ability to land a nice cleanup in ember-cli-fastboot that in turns will unblock fixing a nasty bug in prember that is otherwise unfixable.

[RobbieTheWagner]

I'm hitting issues with this cycle stuff now as well.

[ef4]

We have now discovered a second use case, which is ember-service-worker also suffers the same issue as prember: broccoli-asset-rev needs to run last, and we can't make it run last unless it's capable of rewriting fastboot apps without breaking them.

[RobbieTheWagner]

Just to clarify, this is the ember-service-worker and ember-service-worker-prember issue cycle detected: prember <- ember-cli-fastboot <- broccoli-asset-rev <- ember-service-worker <- ember-service-worker-prember <- prember

[rwjblue]

Published as 2.7.0

[RobbieTheWagner]

🎉