Skip to content

Conversation

@mansona
Copy link

@mansona mansona commented May 14, 2018

This is a work in progress PR that is supposed to spark a discussion around #176. it is also related to kaliber5/ember-fastboot-addon-tests#21

The idea is that we need to have the linking be the last step in the process after all times that npm install is called. As ember-fastboot-addon-tests does some npm magic of its own it would need to run the linking functionality again at a time that is suitable for that addon. I have a demo of what I was trying to achieve kaliber5/ember-fastboot-addon-tests#22

.then(() => this);
};

AddonTestApp.prototype.link = function(appName) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer to call it linkAddon or something like that, to disambiguate it a bit more


function link(appName) {
return Promise.resolve()
.then(() => linkDependencies(appName))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this rather call symlinkAddon() here?

@simonihmig
Copy link
Collaborator

@mansona I am not sure, are the changes in pristine.js really necessary (except for exposing the link function)? AFAICT, it is not calling npm install after symlinking the addon, or do I miss something? It's just ember-fastboot-addon-tests which should call AddonTestApp.link() after doing its own npm install.

Tests are broken at least...

@mansona
Copy link
Author

mansona commented May 15, 2018

@simonihmig you're right in saying that this is mostly for ember-fastboot-addon-tests to do the symlinkAddon() after its own npm install, but that really doesn't work.

To be honest I'm surprised that you haven't come back to say my approach was a million miles off 🤔 I guess if I could get this working "downstream" with these changes I might be able to clean this up and expose symlinkAddon() correctly 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants