Move modifyAssetLocation from build options to the config (for when path option is used)

[robinborst95]

This PR adds a failing test to confirm #344. This test does basically the same as #345 (I copied the snapshot as that should be the exact same), but this test uses the --path option and therefore fails.

[robinborst95]

@thoov @rwjblue , I did an attempt to fix the issue by also looking up modifyAssetLocation in the config. It does fix my added test, but I can imagine you prefer a more generic solution, like always having to define this in the config for example. An even better solution would be having access to the build options from the this.project or this.parent, but I didn't know if and how that is possible. Could any of you shed some light on this?

Edit: I have tested this fix with our codebase and it does indeed work.

[rwjblue]

Hmm. This seems important to do, thank you @robinborst95!

It seems not great that you might have to duplicate, or even that you have to decide that you want to use the config file vs ember-cli-build.js 🤔

[rwjblue]

An even better solution would be having access to the build options from the this.project or this.parent, but I didn't know if and how that is possible. Could any of you shed some light on this?

You could (in your config) instantiate the Project (via require('ember-cli/models/project').closestSync, or we could pass it in as an argument when invoking the config function 🤔

[robinborst95]

It seems not great that you might have to duplicate, or even that you have to decide that you want to use the config file vs ember-cli-build.js

@rwjblue I agree, I think it should work that either the config file or the ember-cli-build file defines the function then. I could find how to get the latter working though. I tried looking into the source code how the build works, and if I understand correctly, the modifyAssetLocation function would be in the lib/broccoli/ember-app's options (so, app.options['ember-cli-code-coverage'].modifyAssetLocation). However, this app is never returned (it's returned as app.toTree()), so I don't see how this can be accessed other than during the build process itself (which, again, happens separately when using the path option, so that's not an option).

The other option of having it in the config file makes a lot more sense to me at all actually, as the modifyAssetLocation function has not so much to do with the build of the files, but instead it has more to do with how/where those files should be reported. Fixing this would be easy, as I have it working already, it just requires some simplification and a modification in the readme.

You could (in your config) instantiate the Project

I don't see how this would help this addon detect the modifyAssetLocation actually, could you elaborate (if still relevant after my explanation of the options above)?

[robinborst95]

@rwjblue @thoov any thoughts on my previous comment?

[rmachielse]

@rwjblue @thoov any chance this could get some attention soon? We are desperate to use the 2.0 beta as it reduces our build time by minutes, but this issue is keeping us from using it.

[rwjblue]

The other option of having it in the config file makes a lot more sense to me at all actually, as the modifyAssetLocation function has not so much to do with the build of the files, but instead it has more to do with how/where those files should be reported. Fixing this would be easy, as I have it working already, it just requires some simplification and a modification in the readme.

@robinborst95 - Let's do it! 😸

[robinborst95]

@rwjblue thanks for your response! I've gone ahead and moved modifyAssetLocation from the build options to the config, and also updated the Readme accordingly. I've tested this in our own app as well and I can confirm it works.

[robinborst95]

Found some typos here while I was at it

[robinborst95]

Removed this as well, as this is a leftover from the 1.0 version

[robinborst95]

@rwjblue @kategengler any chance this PR can be reviewed soon 🙏 ?

[joostverdoorn]

We would love to start using it, thanks @robinborst95 for working on this. @rwjblue @kategengler any chance we can get this merged?

[rmachielse]

@rwjblue @kategengler can you please consider merging this? This has been open for almost a year.

[kategengler]

Unfortunately with the rewrite by @thoov and @rwjblue (which I am grateful for -- it means the package works on modern Ember!), I am not familiar enough with the code to review. Maybe one of them can take a look?