Skip to content
This repository has been archived by the owner on Jan 20, 2019. It is now read-only.

RFC: in-repo addon parity #60

Closed
wants to merge 1 commit into from

Conversation

jamesarosen
Copy link

@jamesarosen jamesarosen commented Jul 16, 2016

@les2
Copy link

les2 commented Jul 24, 2016

Can't wait until this is implemented!

@trentmwillis
Copy link
Member

trentmwillis commented Nov 12, 2016

With Ember Engines gaining adoption, I think figuring out the in-repo-addon story is more crucial than before, especially for testing as we start deploying them to users.

To that end I think the "isolated" testing story makes the most sense. It gives parity with normal addons and encourages developers to avoid coupling their in-repo-addon to their application.

Speaking from experience, the "injected" approach is likely to cause confusion by obfuscating where tests are coming from and further complicates build logic by giving special behavior to in-repo-addons. That said, if this approach is really desired it can be implemented with a simple addon hook override.

I would like to see the command leverage the existing --in-repo flag which some commands already support: ember test --in-repo my-in-repo-addon. I don't believe that this should be entirely difficult to implement.

Unfortunately, there are two important ways in which in-repo addons fail
to meet the goal of isolation:

1. They cannot declare NPM dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a bug, when resolving addons. They resolving code does not take into account the node resolution algorithm. That wont fix this in all cases, but it will help some :)

@hjdivad
Copy link
Contributor

hjdivad commented Nov 14, 2016

I agree with @trentmwillis. There's also a sense in which the isolated strategy is the more general case. After all, the in-repo addon will still be included in the app during its acceptance tests so app specific tests for the in-repo addon could be commingled with the rest of the app's tests. Perhaps that's not as elegant, but it saves the complexity of supporting "injected" and "isolated" style tests for in-repo addons.

It gives parity with normal addons and encourages developers to avoid coupling their in-repo-addon to their application.

This is important, even for addons that are never made public, or external to the repo. Figuring out abstraction boundaries is difficult; we should do what we can to help.

@thedig
Copy link

thedig commented Feb 20, 2017

all roads of in-repo tests leads to this rfc - any update on this as engines pick up steam (har har)?

kevinansfield added a commit to TryGhost/Admin that referenced this pull request Mar 2, 2017
no issue
- removes local addon dependency files to avoid confusion now that the host app manages dependencies
- removes unused tests directory
- these files may come back eventually, there's an open ember-cli RFC to improve in-repo-addon isolation and testing ember-cli/rfcs#60
@trentmwillis
Copy link
Member

@thedig no progress that I am aware of, but I would like to revive this. I wonder if this should take the form of 2 RFCs though, as it is attempting to solve 2 (largely) separate issues. I am particularly interested in the testing story and plan to take a stab at implementing some form of this soon (largely for Engines).

@leizhao4
Copy link

leizhao4 commented Apr 4, 2017

I prefer the "App Author's Choice" option with a flag within each addon/engine's index.js. But the other two options are fine too. I just really want to see this implemented.

@knownasilya
Copy link
Contributor

Would love for this to happen.

@sangm
Copy link

sangm commented Jun 28, 2017

Can we get this back on track?

@jamesarosen
Copy link
Author

jamesarosen commented Jul 20, 2017

In-repo addons don't get linted by ember-cli-eslint because none of the trees that bet passed to eslint include lib/. (The addons could themselves insert their own code into the parent app's lint trees, but that seems like a reasonable default behavior.)

@tmquinn
Copy link

tmquinn commented Oct 1, 2018

So is this any better than having a repo that looks like this:

+--my-host-app/
   +--lib/
      +--regular-in-repo-engine/    
+--my-external-engine/

If you want an external like engine, in the same repo, does it need to be an in-repo-engine?

@rwjblue
Copy link
Member

rwjblue commented Jan 19, 2019

We are working on closing the ember-cli/rfcs repo in favor of using a single central RFC's repo for everything. This was laid out in https://emberjs.github.io/rfcs/0300-rfc-process-update.html.

Sorry for the troubles, but would you mind reviewing to see if this is still something we need, and if so migrating this over to emberjs/rfcs?

Thank you!

@rwjblue rwjblue closed this Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.