Filter npm module defintions from catalogEntriesByType #162
Conversation
Don't return things from catalogEntriesByType that don't belong to your main app.
|
This is not a viable fix, unfortunately. The current implementation pulls in everything from
Unfortunately both of those are hard. |
|
|
||
| test('Ignores browserify modules', function(assert) { | ||
| assert.expect(1); | ||
| def('npm:cool/model/package'); |
There was a problem hiding this comment.
This test will never fail. model needs to be pluralized.
There was a problem hiding this comment.
Confirm on this one. Should be models here...
|
|
||
| test('Ignores modules that are not part of the main app', function(assert) { | ||
| assert.expect(1); | ||
| def('liquid/awesome/model'); |
There was a problem hiding this comment.
Pluralize, add trailing slash.
There was a problem hiding this comment.
@nathanhammond - This particular module stub is emulating "pod models" (your favorite 😉 ), and singular form is correct here.
|
@nathanhammond do you have an example for where this fix fails? We originally filtered out things starting with |
|
@igorT Given this if you have an addon which defines a model which ended up at Forcing everything to be in the application namespace is too heavy-handed. (Note: I prefer this change, but I don't believe that we can reasonably make it except as part of something like this: emberjs/rfcs#170) |
FWIW, this is untrue. Models that are not in the applications namespace can never be resolved anyways (since we only resolve things in the apps namespace). |
|
|
||
| test('Ignores modules that are not part of the main app', function(assert) { | ||
| assert.expect(1); | ||
| def('liquid/awesome/model'); |
There was a problem hiding this comment.
@nathanhammond - This particular module stub is emulating "pod models" (your favorite 😉 ), and singular form is correct here.
|
|
||
| test('Ignores browserify modules', function(assert) { | ||
| assert.expect(1); | ||
| def('npm:cool/model/package'); |
There was a problem hiding this comment.
Confirm on this one. Should be models here...
|
@rwjblue / @ryanlabouve / @nathanhammond This is on the right track or... (above conversation was a tad confusing) @ryanlabouve do you have any cycles to address @rwjblue concerns? That way we can move this forward. |
|
Sure thing! Sorry for delay. Will do it tonight. |
|
Yep, this is definitely going the right direction. I think I only had a couple of minor changes needed to land it... |
|
This code actually now resides within ember itself: https://github.com/emberjs/ember.js/blob/aeeddf7c00b42a340494465252481b5414d87009/packages/ember-extension-support/lib/container_debug_adapter.js (since ember 1.7, but this library accidentally overrode it for compat). This fix should likely land there, we can always backport here after (for the 2x series) |
I created this PR to help explain the issue I reported here: #161
I understand that this may not the the desired solution. However if this is useful, or if there's some other direction I can take to help solve this problem, I'm happy to do so. Just let me know :)