-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Update plugin API #1196
Update plugin API #1196
Conversation
585b7a1
to
ad6d51b
Compare
ad6d51b
to
2cfd1ee
Compare
src/app/plugin/pluginManager.js
Outdated
data.arguments.push((error, result) => { | ||
response(data.type, data.id, error, result) | ||
data.value.push((error, result) => { | ||
response(data.key, data.type, data.id, error, result) | ||
}) | ||
api[data.key][data.type].apply({}, data.arguments) |
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.
data.arguments
should be data.value
test-browser/plugin/remix.js
Outdated
@@ -8,25 +8,31 @@ window.addEventListener('message', receiveMessage, false) | |||
window.onload = function () { | |||
document.querySelector('input#testmessageadd').addEventListener('click', function () { | |||
window.parent.postMessage(JSON.stringify({ | |||
action: 'request', | |||
key: 'editor', |
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.
key: 'editor'
should be key: 'config'
test-browser/plugin/remix.js
Outdated
}) | ||
|
||
document.querySelector('input#testmessageremove').addEventListener('click', function () { | ||
window.parent.postMessage(JSON.stringify({ | ||
action: 'request', | ||
key: 'editor', |
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.
key: 'editor'
should be key: 'config'
test-browser/plugin/remix.js
Outdated
}) | ||
|
||
document.querySelector('input#testmessagerget').addEventListener('click', function () { | ||
window.parent.postMessage(JSON.stringify({ | ||
action: 'request', | ||
key: 'editor', |
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.
key: 'editor'
should be key: 'config'
* type: <string>, | ||
* value: <array>, | ||
* error: (see below) | ||
*} |
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.
I just checked again: https://github.com/ethereum/remix-ide/issues/1156
- The original proposal is
var { id, status, type, key, value } = msg
- You said you would like to add the
action
field to indicate "response/notification/request". - That would make it
var { action, id, status, type, key, value } = msg
- But now the
status
is/was renamed toerror
i guess.
Just to make sure how i have to think about it :-)
So i guess we now have: var { action, id, error: status, type, key, value } = msg
I would say that is fine.
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.
The above comment is just for me to clarify.
If this is how it's meant, i would say the PR looks good to me :-)
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.
yes except that error is undefined if there's no error
following https://github.com/ethereum/remix-ide/issues/1156