Fix broccoli rebuild

[simonihmig]

The manifest was rewritten on each incremental build. As a side effect, CSS hot reloading was not working, the browser would always do a full reload!

Note: I updated engine to >= 4, so we can use some ES6. Hope that's ok!

[simonihmig]

Hm, those failing tests seem to not be related, same on master... 🤔

[san650]

@simonihmig Thanks for fixing this! I had no idea how to solve this issue. My only concern is, when the manifest.js file is edited, does it triggers a rebuild of the manifest.json file? I ask because one of the shortcuts I took was not to define any input tree on the broccoli plugin call.

See https://github.com/simonihmig/ember-web-app/blob/7cbafee801f85f1225b7f5e8de5c7359a741a75b/lib/broccoli/generate-manifest-json.js#L11

If this is a problem I think we can workaround it by adding config/ to the input nodes and changing a bit the plugin.

Hm, those failing tests seem to not be related, same on master... 🤔

I'm having a hard time with ember-cli-addon-tests + travis-ci. The tests run fine locally but for some reason they started to fail on travis. I have to look into that later.

[simonihmig]

I had no idea how to solve this issue.

The changes basically reflect what is happening here: https://github.com/rwjblue/broccoli-file-creator. Actually we could have used that as a dependency, but as it was just a couple of lines, I just added the missing pieces...

does it triggers a rebuild of the manifest.json file

Hm, I guess it does not! Didn't think about that... if we don't have the manifest.js as an input node, it is probably not watched!? I guess we need to figure that out!

I'm having a hard time with ember-cli-addon-tests + travis-ci. The tests run fine locally but for some reason they started to fail on travis.

Ok, I can take a look too!

[san650]

@simonihmig I'm Okay with merging this PR as it is and working on code reloading on a different PR. Let's open a ticket for that work so we can track the missing pieces.

If you're busy to implement the fix I can try tackling it myself next week.

What do you think?

[simonihmig]

What do you think?

Ok for me! I do think breaking CSS hot reloading is worse than reloading of the manifest not working. I was just a bit hesitant, because releasing this would still some kind of breaking change...

The special case with the manifest is probably that we actually have to require and evaluate the code, rather than just moving/transforming this in some way with broccoli. I guess it is quite similar to how config/environment.js works. But as this is supporting reloads, there should be a solution. Maybe we can see how that is solved in ember-cli?

[simonihmig]

@san650 are you sure that regenerating the manifest without running a new ember s/ember b was ever working? Just tried this with the master branch in an existing app, and does not seem to work for me. It rebuilds the app because of changes in manifest.js, but the resulting manifest.json did not reflect the changes for me!

Actually when looking at the code, this seems to be how it is. Because the actual manifest data seems to be read in the treeForPublic hook, and that is only executed once, not for every rebuild, afaik.

I have an idea how to make true dynamic rebuilding of the manifest work, without breaking CSS hot reloading, but if this was not working before, I think we can indeed safely merge this (as we don't introduce any new regression) and work on the rebuild thing in a separate PR!?

[san650]

@simonihmig thanks for taking the time to look into this.

You're correct in that I was assuming that the rebuild was working, but if it wasn't then we can merge this PR safely.

Sorry for all the troubles!