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

Add generic options object to App init #46

Merged
merged 4 commits into from
Jun 3, 2020
Merged

Conversation

divyaashritha
Copy link
Contributor

Summary

Add generic options object to application initialization so that app can send this optional object to frame on authorization.

@@ -251,7 +252,7 @@ class Application extends EventEmitter {

// Emit a ready event
this.emit('xfc.ready');
this.JSONRPC.notification('authorized', [{ url: window.location.href }]);
this.JSONRPC.notification('authorized', [{ url: window.location.href, options: this.options }]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we always send options or should we only send if it has something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it non-passive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is just extra info object always present/received, it could be read or ignored.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no problem with always sending it then

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is any work required to make this available on the Consumer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need any changes on Consumer as we are merging in the new options object with url into one detail object. https://github.com/cerner/xfc/blob/master/src/consumer/frame.js#L51

@mhemesath
Copy link
Contributor

Please add documentation for this feature

@@ -17,9 +17,10 @@ class Application extends EventEmitter {
* This string must be a valid CSS selector string; if it's not,
* a SyntaxError exception is thrown.
*/
init({ acls = [], secret = null, onReady = null, targetSelectors = '' }) {
init({ acls = [], secret = null, onReady = null, targetSelectors = '', options = {} }) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add documentation for this new parameter.

@@ -1,5 +1,8 @@
Next Release
-------------
1.8.2
------
* Add generic options object to App init. #46
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be a 1.9 feature release. Also, we need README documentation on how to use this new feaature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will update it.

@divyaashritha divyaashritha merged commit 8bc080f into master Jun 3, 2020
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

Successfully merging this pull request may close these issues.

4 participants