Make Port#uniqueId a bit more resilient. #929
Conversation
Fixes app detection issues by using a layered fallback system to control the uniqueId: * Use `name` specified in `app/app.js` (the easiest for users to customize to control how their app is displayed in the inspector) * Use `modulePrefix` specified in `app/app.js` (all ember-cli apps would have this, and is _generally_ still representative of what each app owner calls their app). * Use `(unknown app - <RANDOM_GUID>)` as a last chance fallback to ensure **all** detected applications at least show up
| @@ -13,8 +13,20 @@ export default EmberObject.extend(Ember.Evented, { | |||
| * @property uniqueId | |||
| * @type {String} | |||
| */ | |||
| uniqueId: computed('namespace._application.name', function() { | |||
| return this.get('namespace._application.name'); | |||
| uniqueId: computed('namespace._application.{name,modulePrefix}', 'namespace.applicationId', function() { | |||
There was a problem hiding this comment.
As we discussed briefly in Discord, I think this should be an applicationName computed, and we should revert to the old uniqueId generation. We then need to pass both the name and id around and look up the apps by the id somehow, rather than name, I think.
There was a problem hiding this comment.
Sounds good to me @rwwagner90, I'll leave that to you (happy to review and otherwise help out as needed though)...
There was a problem hiding this comment.
As we discussed briefly in Discord, I think this should be an
applicationNamecomputed, and we should revert to the olduniqueIdgeneration. We then need to pass both the name and id around and look up the apps by the id somehow, rather than name, I think.
This makes sense to me as well. I was going to leave a comment to this effect after I thought about the idea of doing something like app${randomNum} last night, but then realized that probably wouldn't work for app lookups anymore when switching between apps.
There was a problem hiding this comment.
@alexhancock I started trying to do this, but I've been so far unsuccessful. Maybe you could take a look and help get this over the finish line? #930
There was a problem hiding this comment.
@alexhancock are you on Discord? It would be good to sync up there.
There was a problem hiding this comment.
Cool! I see it now, but I don't have permissions to send messages in the dev-ember-inspector channel
There was a problem hiding this comment.
Heading into a meeting for an hour, but I really only have one question about the changes in #930 so I will just post it here! :P
Why all the changes to the logic in app/port.js and why remove the sendAppList logic?
As far as I can see that code being altered/removed is connected to why populating the list of apps and switching apps no longer works in that changeset. Are the changes born from a desire to clean things up or implement the original app list population logic in a different way?
Happy to help on a revised version of this change after my lunch meeting.
There was a problem hiding this comment.
@alexhancock the app list was using the uniqueId and the names as the same thing. We need a reworking of it to support sending both applicationId and applicationName, like the PR is doing. We already have a for loop that loops through the applications, so if we can find a way to use that to populate the apps, that would be ideal. Ping me on discord I am rwwagner90 there as well. I see two alexhancock so I am not sure which is you. You have to go through the #welcome and #setup-profile channels to get permissions to everything.
There was a problem hiding this comment.
Also, another option would be to revert my original change, release a new version and then re-introduce the changes once we get the issues resolved.
Not sure how close we feel we are on fixing the issues - I'll leave it up to you.
There was a problem hiding this comment.
@alexhancock let's discuss via discord, it will make things quicker
Fixes app detection issues introduced in #898 by using a layered fallback system to control the uniqueId:
namespecified inapp/app.js(the easiest for users to customize to control how their app is displayed in the inspector)modulePrefixspecified inapp/app.js(all ember-cli apps would have this, and is generally still representative of what each app owner calls their app).(unknown app - <RANDOM_GUID>)as a last chance fallback to ensure all detected applications at least show upFixes #927