Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fallback to project if addon has no root #71

Merged
merged 3 commits into from
Jan 9, 2019

Conversation

buschtoens
Copy link
Contributor

Fixes #70.

@ef4
Copy link

ef4 commented Nov 26, 2018

Thanks @buschtoens, I'm using this patch to workaround the bug for ember-auto-import's test suite (which is now a monorepo, so I can more easily test both babel 6 and babel 7 based apps).

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to land this (or fix @ember-decorators/babel-transforms), would you mind adding a previously failing test?

@alexlafroscia
Copy link

Would it make more sense for the EmberAddon object for the dummy app to have a root property instead of working around that quirk here?

@buschtoens
Copy link
Contributor Author

If the notion of root is applicable to an EmberAddon, this would also be a nice fix, I guess. However, that would require affected users to upgrade their host ember-cli version. So maybe we'd wanna do both?

@buschtoens
Copy link
Contributor Author

@rwjblue I've added a test for the addon scenario:

falls back to the project root for instances of EmberAddon that do not have a root property

I wanted to add a real life workspaces scenario as well that explicitly checks that resolve.sync does not fall back to the caller file's physical location, but that is not possible with the current test harness, since the caller file is the test itself.

Let me know, if you want me to change anything.

@rwjblue
Copy link
Member

rwjblue commented Jan 7, 2019

but that is not possible with the current test harness, since the caller file is the test itself

I've just landed some new tests that could show how to do this...

@buschtoens
Copy link
Contributor Author

buschtoens commented Jan 8, 2019

@rwjblue Thanks for providing such a great basis and pushing for good tests! I've added a real life workspaces scenario.

Want me to change something?

@rwjblue rwjblue merged commit 0cdc745 into ember-cli:master Jan 9, 2019
@rwjblue
Copy link
Member

rwjblue commented Jan 9, 2019

Thank you @buschtoens!

@rwjblue rwjblue added the bug label Jan 9, 2019
@rwjblue
Copy link
Member

rwjblue commented Jan 9, 2019

ember-cli-version-checker 3.0.1 published 🎉

@buschtoens buschtoens deleted the fix/70-yarn-workspaces-dummy branch January 9, 2019 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants