Icon pack resolution breaks when dependencies not aligned

EWhite613 commented 6 years ago

Summary

Icon pack resolving svg path doesn't work well with basedir: this.project.root when dependecies aren't aligned

Expected Behavior

Should've resolved to addon's dependency

Actual Behavior

Always resolves to projects top level dependency version

Possible Solution

Align dependency version

Steps to Reproduce

Have ember-frost-core@7.0.0
Nested have ember-frost-core@8.2.5
- will see ENOENT: no such file or directory, lstat '/workspace/mcp-ui/node_modules/ember-frost-core/frost-icon-svgs'
- Since 7.0.0 stores svgs in svg folder, and 8.2.5 stores in frost-icon-svgs

notmessenger commented 6 years ago

This is not an issue that needs to be addressed via changes to this codebase. What needs to happen is for packages depending on older versions of this addon, which might be used alongside newer versions of the same, to be updated so that all code and dependencies are in sync.

sophypal commented 6 years ago

While @notmessenger has a point in that the root of the problem is not being fixed, I still think this issue being address in this code base is still valid. It ensures that the icons are being fetched from the right repo vs whatever winner comes out of the npm alignment issue.

At least with this work, we can limp along until we can address the huge fallout of of multiple versions in our repo.

notmessenger commented 6 years ago

This change requires the following scenarios to be tested:

Test that each addon that uses this addon to make its svg packs continues to function as expected.
- For those that are using the same code to do so it should be a "rubber-stamp" review but there is no guarantee that they are all being done the same.
An addon that consumes another addon that makes its own svg pack needs to be tested
The above combinations then also need to be tested when consumed in an application.
The above combinations then also need to be tested in an addon like frost-foundation that is consumed by an application.
For good measure test that all of this works against existing apps as well, such as MCP-UI.

This code is very intertwined with how Ember CLI works and the complicated relationship we have between all of our addons and these are all of the testing steps/scenarios we had to employ when making the last round of refactors. Without this testing there is no guarantee that something does not become broken.

When we mentioned this to @EWhite613 he said that he did not have the time to do all of this testing.

This comment (thread) serves as my due diligence in laying out the concerns with this approach so if @cstolli says to merge https://github.com/ciena-frost/ember-frost-core/pull/579 then we should proceed.

juwara0 commented 6 years ago

I know that the level of testing that @notmessenger outlined was conducted and verified that functionality was working correctly prior to this change: https://github.com/ciena-frost/ember-frost-core/pull/577/files

sophypal commented 6 years ago

Great points guys. I think it's fair that we do at least that much testing to ensure this doesn't break anything else. I would prefer that @cstolli not merge this without that testing because I'm not familiar with the code to be confident enough to give my approval. We're not dead in the water because there is a work around.

ciena-frost / ember-frost-core