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: avoid assuming host app has ember-cli-babel installed #368

Merged
merged 1 commit into from
Jan 19, 2021

Conversation

alexlafroscia
Copy link
Contributor

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 accessing pkg on undefined.

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.

Definitely seems good to me, would you mind adding a test to make sure we don't regress again?

@alexlafroscia
Copy link
Contributor Author

alexlafroscia commented Nov 18, 2020

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 ember-cli-babel as a dependency but an app that does not. Does that sound right?

https://github.com/babel/ember-cli-babel/blob/master/node-tests/addon-test.js

@rwjblue
Copy link
Member

rwjblue commented Nov 18, 2020

@alexlafroscia
Copy link
Contributor Author

Awesome! I'll take care of that soon. Thanks for some help planning out how to test this!

@rwjblue
Copy link
Member

rwjblue commented Nov 19, 2020

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.

@alexlafroscia alexlafroscia force-pushed the avoid-host-app-assumption branch from 729641e to 85eb28a Compare November 22, 2020 17:48
@alexlafroscia
Copy link
Contributor Author

I went and rebased from master to make sure this still works with the PR @rwjblue mentioned above merged.

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!

@rwjblue
Copy link
Member

rwjblue commented Jan 13, 2021

Dang, looks like we have another conflict. Sorry for letting this linger @alexlafroscia... 😞

@rwjblue rwjblue force-pushed the avoid-host-app-assumption branch from 85eb28a to 039f812 Compare January 19, 2021 20:17
@rwjblue
Copy link
Member

rwjblue commented Jan 19, 2021

Did the rebase locally...

@rwjblue rwjblue added the bug label Jan 19, 2021
@rwjblue rwjblue force-pushed the avoid-host-app-assumption branch from 039f812 to c993d65 Compare January 19, 2021 20:19
@rwjblue rwjblue merged commit 7687690 into emberjs:master Jan 19, 2021
@alexlafroscia
Copy link
Contributor Author

Sorry I didn't get to this before you did. Thanks for re-basing this PR for me and getting it merged @rwjblue! 🙌

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.

2 participants