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

Browserify modules included in catalogEntriesByType('model'), which throws error downstream #161

Closed
ryanlabouve opened this issue Oct 12, 2016 · 3 comments

Comments

@ryanlabouve
Copy link

So, quick disclaimer: I'm not 100% sure this is an issue with resolver. This issue is also a little dense to explain clearly, so I'm attaching a PR which may help demonstrate / fix the issue.

When I run catalogEntriesByType('model') (specifically from data_adapter.js#L354), browserify modules that have the word "model" in their module definition are included in the returning array.

This is causing a ember.js to throw an error later when it tries to lookup a module like npm:my/cool/model via data_adapter.js#L362, essentially resulting in something like getOwner(this)._lookupFactory('model:npm:my/cool/model') via this which throws Error message: Invalid Fullname, expected: 'type:name' got: model:npm:prosemirror/dist/model.

Please note above, the issue is specifically caused by the lookup trying to add model: on the the front of the existing package name which makes it violate the naming constraints of a module.


In case you are wondering how I ran into this, the Data tab on Ember Inspector breaks when we have a NPM package installed via Ember Browserify that has the word "model" in the module definition.

ryanlabouve added a commit to ryanlabouve/ember-resolver that referenced this issue Oct 12, 2016
@igorT
Copy link

igorT commented Nov 30, 2016

@ryanlabouve @rwjblue said the correct approach would be to only lookup modules that start with the modulePrefix instead of filtering out npm

@ryanlabouve
Copy link
Author

ryanlabouve commented Dec 1, 2016

Sweet! @igorT and I paired to do this. The attached PR has the latest changes.

@kategengler
Copy link
Member

Closing due to age.

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

No branches or pull requests

3 participants