Conversation
…oad not yet working)
…settings page so they can enable it
…r when disabling a command that is already disabled
… are officially released
|
|
geekgonecrazy
left a comment
geekgonecrazy
left a comment
There was a problem hiding this comment.
Pass one :)
Maybe add a README.md to base and define what Orchestrator, bridge and a few other terms specific to apps?
| if (got.info && got.logs) { | ||
| this.ready.set(true); | ||
| } | ||
| }); |
There was a problem hiding this comment.
What happens if these both finish at the same time? Maybe should wrap this in something that will catch both of these like Q.all ?
Because what if they finish at the same time I think extremely tiny chance, but might happen that neither execute: this.ready.set(true)
|
|
||
| got.settings = true; | ||
| if (got.info && got.settings) { | ||
| this.ready.set(true); |
There was a problem hiding this comment.
Same with these 2 parallel promises
| this.orch = orch; | ||
| this.streamer = new Meteor.Streamer('apps'); | ||
|
|
||
| this.streamer.on('app/added', this.onAppAdded.bind(this)); |
There was a problem hiding this comment.
if you define these as arrow function you won't have to do .bind(this) because the arrow functions don't create new scope.
onAppAdded = (appId) => {
}
Not really a change request. Just could avoid having to do this unfortunate bind junk :)
There was a problem hiding this comment.
As that's a es7 feature and we are using eslint for es6, that style of development isn't possible. Also, the default scope of this in class functions is the class however the Meteor.Streamer messes with the scope and thus is what required the additional .bind(this) on setting the streamer callbacks.
There was a problem hiding this comment.
As per conversation -- due to something in meteor and/or the streamer we can use arrow functions there
…ition. Add a readme explaining a few terms
|
I think i've said this before.... But: Really like the simplicity of using the streamer for listening for server side events. Makes it much simpler to understand the flow of data. |
|
|
||
| this._addAdminMenuOption(); | ||
|
|
||
| const loadLangs = setInterval(() => { |
There was a problem hiding this comment.
Hm... is there another way other than setting an interval? I thought meteor had a way to add a callback when its ready.
| 'Accounts_OAuth_Wordpress_secret', 'Push_apn_passphrase', 'Push_apn_key', 'Push_apn_cert', 'Push_apn_dev_passphrase', | ||
| 'Push_apn_dev_key', 'Push_apn_dev_cert', 'Push_gcm_api_key', 'Push_gcm_project_number', 'SAML_Custom_Default_cert', | ||
| 'SAML_Custom_Default_private_key', 'SlackBridge_APIToken', 'Smarsh_Email', 'SMS_Twilio_Account_SID', 'SMS_Twilio_authToken' | ||
| ]; |
There was a problem hiding this comment.
We should probably add a todo, to add a way to mark a setting as containing a secret, that way could just block access to settings containing secrets. Long term maintaining this internal list... not fun
There was a problem hiding this comment.
New issue created to reflect this: #9790
| instance.settings.set(results[1].settings); | ||
| this.ready.set(true); | ||
| }).catch(() => { | ||
| //TODO: error handling |
There was a problem hiding this comment.
Can we just toss an error for now? That way if this is hit during testing, we at least know where it was?
6 participants
Adds the initial not fully implemented Rocket.Chat Apps. We are intentionally not including a lot of information about this pull request and the features it adds. Mostly because they are disabled by default, can not be enabled unless you add the right things to the environment (:wink:), and are a very much work in progress. Why are we adding it then? So that the people who are wanting to use it and know about it can test it and so we can test it before recommending it for production usage.
If you would like more information about this new feature and the plan for it, please contact me on Rocket.Chat's Open Community server. :)
cc: #6890