manifest-inserter causes index.html to show up as changed on every rebuild about ember-asset-loader HOT 21 CLOSED

Closed by #59, released as 0.4.1.

from ember-asset-loader.

Definitely sounds like a bug. Would also explain the reports we've received from ember-engines users about full reloads being triggered on CSS changes.

from ember-asset-loader.

Did you folks find a solution for this? Anything I could do to help? It'd be great to improve the workflow with engines that this would enable

from ember-asset-loader.

@mfeckie Still an issue AFAIK. I assume that @trentmwillis and team would welcome a fix.

from ember-asset-loader.

No workarounds really. I won't be getting to this anytime soon, but would gladly welcome a PR attempting to fix.

from ember-asset-loader.

@trentmwillis So I've spent a little time trying to figure out how to help with this and I'm not sure how to proceed.

It seems like the issue is around this https://github.com/ember-engines/ember-asset-loader/blob/master/lib/manifest-generator.js#L16-L27

Because the placeholder is always %GENERATED_ASSET_MANIFEST%, this cause the cache to always be busted from the perspective of BroccoliCachingWriter.

I'm prepared to put some work into a way to resolve this, but need some input on what approach should be taken.

It feels like there's a different flow for a development environment over production.

In development, it seems like there needs to be a better way of say 'the bundle has changed' and then triggering an update of the meta tag. Something along the lines of what happens with live reload now for CSS seems like it could be good, but that wouldn't likely trigger the loaders from this addon.

Thoughts?

from ember-asset-loader.

Hmm, I was thinking that the fix would roughly be to keep track of the last written value and only call fs.writeFileSync(path.join(this.outputPath, this.options.indexName), index); (around here) if the actual contents changed.

from ember-asset-loader.

So, that was what I tried

  if (existsSync(indexFilePath)) {
    var indexFile = fs.readFileSync(indexFilePath, { encoding: 'utf8' });
    var index = this.replacer(indexFile, appTransformedManifest);
    if (indexFile !== index) { 
       fs.writeFileSync(path.join(this.outputPath, this.options.indexName), index);
    }

But as far as I can tell, the contentFor hook overwrites it (in indexFile) with %GENERATED_ASSET_MANIFEST% and so they never match.

But perhaps there's some better way of doing that?

Perhaps storing the lastWritten value on this ..

from ember-asset-loader.

Hmm, @mfeckie brings up a good point re: the contentFor hook. I'm unfamiliar with how this all works in practice (haven't done much addon development aside from various PRs), so I'd best start with a question: will this hook get executed on each reload? If so, perhaps what's happening is something like this:

• SCSS is modified
• Live Reload starts
• The contentFor hook gets executed, inserting the placeholder record (and busting the cache)
• Placeholder is replaced with actual manifest definition later

...which means the real trouble is that contentFor is throwing a spanner in the works. We'd have to do a no-op there, but that's too early in the lifecycle to tell whether or not the manifest has actually changed.

Again, dunno if that's how it all works in practice; throwing it out there just in case.

[SEMI-EDIT] Took a while for me to type this out; the post directly above seems to be evidence of this. Hmm.

from ember-asset-loader.

From my current testing, that is indeed the case. The contentFor hook is causing an issue.

Will spend a little more time futzing around

from ember-asset-loader.

Sorry, no I'm suggesting something more like master...cache-rebuilds. Not sure if that works, just showing what I was thinking.

from ember-asset-loader.

^ may require changing the base class to just broccoli-plugin and managing the caching ourselves (which is roughly what it is doing anyways)....

from ember-asset-loader.

Yep @rwjblue

  if (existsSync(indexFilePath)) {
    var indexFile = fs.readFileSync(indexFilePath, { encoding: 'utf8' });
    var index = this.replacer(indexFile, appTransformedManifest);

    if (index !== this.lastIndex) {
      this.lastIndex = index;
      fs.writeFileSync(path.join(this.outputPath, this.options.indexName), index);
    }

  }

This is what I was trying just before your follow up, but it still triggers a full reload. The issue seems to be that the contentFor hook firing.

Will try with plain broccoli-plugin though

from ember-asset-loader.

So, even converting to broccoli-plugin still trigger full rebuild.

I suspect this is why

asset_manifest_inserter-output_path-k3JcBhyz.tmp -> index.html
has

    <meta name="reproduce-asset-loader-issue/config/asset-manifest" content="%GENERATED_ASSET_MANIFEST%" />

and

asset_manifest_inserter-output_path-k3JcBhyz.tmp
has

<meta name="reproduce-asset-loader-issue/config/asset-manifest" content="%7B%22bundles%22%3A%7B%7D%7D" />

So I think this is resulting in Broccoli reporting back that something has changed?

from ember-asset-loader.

Can you push a branch with your change to broccoli-plugin?

from ember-asset-loader.

Sure https://github.com/mfeckie/ember-asset-loader/tree/bugfix/avoid-triggering-reload there you go!

from ember-asset-loader.

What I've pushed fails on a rebuild due to the meta tag going back to
<meta name="reproduce-asset-loader-issue/config/asset-manifest" content="%GENERATED_ASSET_MANIFEST%" />

  Plugin.call(this, inputNodes, {
    annotation: options.annotation,
    persistentOutput: true
  });

Setting persistentOutput stops the failure, but doesn't solve the refresh issue

from ember-asset-loader.

Right, I think we need persistentOutput and the thing I attempted in my branch linked above. Basically, persistentOutput prevents broccoli from automatically clearing the output folder, and the caching prevents us from modifying the output folder if the values are actually the same. This should result in no work on rebuilds (and therefore no downstream file changes, and therefore no full page reloads for only .css file changes)...

from ember-asset-loader.

I've pushed onto that branch including persistentOutput, but it still triggers full page refresh.

I still suspect it's caused by the difference between the input and output tmp folders.

from ember-asset-loader.

OK, got it working, PR inbound :)

from ember-asset-loader.

Thanks so much for tracking this issue down everyone! 👏 😄

from ember-asset-loader.