-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
[externalComponent] Add persistence support for external component #15223
[externalComponent] Add persistence support for external component #15223
Conversation
shiqimei
commented
Aug 21, 2019
•
edited
Loading
edited
* add currentExternalComponent to maintain the opened externalComponent info
Hey @lolimay , apparently the file |
import { Meteor } from 'meteor/meteor'; | ||
|
||
export function setItem(appId, key, value) { | ||
Meteor.call('externalComponentStorage:setItem', appId, key, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're moving from Meteor Methods to REST endpoints, in an effort to minimize our dependency on Meteor. Please change this approach accordingly :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Will re-implement it with REST API.
Yeah, I moved |
Oh, I see! It's ok, then :) |
import { setItem, getItem, getAll, clear, removeItem } from './persistence'; | ||
|
||
Meteor.startup(function() { | ||
window.addEventListener('message', async ({ data, source }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a comment here, we can work on that later on.
I think this part of the code should live in the Apps-Engine, as a AppsEngineUIHost
or something similar. Then, we can keep the communication logic in one repo and make sure they are compatible for every Apps-Engine version.
As it is currently implemented, apps would have not only a dependency to the Apps-Engine version, but a dependency to Rocket.Chat's version as well, and things will begin to get "messier", harder to maintain.
Of course, if we move this part into the Apps-Engine, we are going to need some dependencies to be injected in the AppsEngineUIHost
class, as it won't have easy access to Meteor
variable, for instance, similarly to the bridges
used for the server side of the engine. But we are going to benefit from moving away from tight coupling to the Meteor
framework, which is a big plus :)
* implemented with REST endpoints now
The related changes in the Rocket.Chat.Apps-engine repo is here: RocketChat/Rocket.Chat.Apps-engine#135 |