-
-
Notifications
You must be signed in to change notification settings - Fork 119
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: avoid assuming host app has ember-cli-babel
installed
#368
fix: avoid assuming host app has ember-cli-babel
installed
#368
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely seems good to me, would you mind adding a test to make sure we don't regress again?
Happy to! Could you point me in a direction toward how I could do that? My guess would be some kind of new test case in this file, that configures a faux addon that has https://github.com/babel/ember-cli-babel/blob/master/node-tests/addon-test.js |
Yes, exactly. You'd make a test for the function in this section: Similar to this test: But just before the expectation, set |
Awesome! I'll take care of that soon. Thanks for some help planning out how to test this! |
Just a heads up, I just merged #367 which moves the files that I linked to above around a little bit. It shouldn't change any of the specifics though. |
729641e
to
85eb28a
Compare
I went and rebased from I made sure the test I added actually fails if I don't include the "fix" that is included as well. It fails without it and passes with it! |
Dang, looks like we have another conflict. Sorry for letting this linger @alexlafroscia... 😞 |
85eb28a
to
039f812
Compare
Did the rebase locally... |
039f812
to
c993d65
Compare
Sorry I didn't get to this before you did. Thanks for re-basing this PR for me and getting it merged @rwjblue! 🙌 |
The original code here assumes that searching for
ember-cli-babel
in the host app's addons always has a "hit". However, this might not be the case, if the host app is trying to experiment with some other transpiler.The change here allows the for lookup of the package version in the host app's dependencies to return
undefined
without an error related to accessingpkg
onundefined
.