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

Support plugin only request during plugin initialization #202

Closed
wants to merge 1 commit into from

Conversation

ben181231
Copy link
Contributor

connect #189

@oursky-travis
Copy link

@ben181231, thanks for your PR! By analyzing the history of the files in this pull request, we identified @cheungpat to be a potential reviewer.

@cheungpat
Copy link
Contributor

Sorry this issue comes to my attention just now.

It appears that this PR does not work similar to the counterpart in py-skygear. In py-skygear, the _from_plugin parameter is added if the plugin specifically specify “plugin_request” when calling send_action. But this PR suggest to send _from_plugin=true for all requests.

I’d suggest that we keep the behaviour of py-skygear in here because I think the developer need to be explicit they are overriding the server safeguard to prevent API calls before all plugins are initialized.

@ben181231
Copy link
Contributor Author

I’d suggest that we keep the behaviour of py-skygear in here because I think the developer need to be explicit they are overriding the server safeguard to prevent API calls before all plugins are initialized.

I also think it is good to keep the behavior as py-skygear. However, the CloudCodeContainer has so many methods like signupWithUsername(), loginWithEmail(), etc. that call sendRequestObject(). If we need to make the "plugin only" request explicit, I can come up with the following solutions:

  1. Override all the methods in container that call sendRequestObject() behind;
  2. Create a instance method like pluginOnlyRequest() that clone the whole container with a flag like this.sendPluginOnlyRequest set to true.

I think the solutions above are not that good. Consider that, always setting _from_plugin is acceptable?

@cheungpat

@cheungpat
Copy link
Contributor

I actually expect cloud code people to get a new instance of CloudCodeContainer every time they need to use it. Is that not the case?

Since API requests are constructed by Container, and _from_plugin is one information for constructing a request that could apply to all actions, I think it is logical to add this information to a request by an instance of Container (just like API key, user ID etc). Therefore, I would suggest something like (2).

I imagine the behavior would be similar to apiKey, endPoint, asUserId etc.

Is this not right? I also understand that it seems logical in JS to return a cloned copy of CloudCodeContainer like exactly what you mentioned in (2). But this pattern is not followed by other properties like apiKey, endPoint and asUserId.

@chpapa
Copy link

chpapa commented May 2, 2017

I actually expect cloud code people to get a new instance of CloudCodeContainer every time they need to use it. Is that not the case?

Reminds me of this: #186 relevant?

@ben181231
Copy link
Contributor Author

cloud code people to get a new instance of CloudCodeContainer every time they need to use it

If we all agree on this, let's move the option to constructor. it would be better to clone the whole object.

@cheungpat
Copy link
Contributor

cloud code people to get a new instance of CloudCodeContainer every time they need to use it

If we all agree on this, let's move the option to constructor. it would be better to clone the whole object.

@rickmak

@rickmak
Copy link
Member

rickmak commented May 4, 2017

cloud code people to get a new instance of CloudCodeContainer every time they need to use it

I think we can take this assumption. But we need to change the CloudCodeContainer behave as in py-skygear SkygearContainer. i.e. const containerWithDefaultConfig = new CloudCodeContainer().config();

If we all agree on this, let's move the option to constructor. it would be better to clone the whole object.

The constructor is sync, while the configuration of the container is async. We cannot do this.

Override all the methods in container that call sendRequestObject() behind;

This is not preferable. The sendRequestObject should look up the container state.

======
As per the workflow, we need to write the usage doc at the same time as the implementation?

My take on the example usage code:

// This will give the container with default config. i.e. in cloud deployment, config like endPoint and apiKey are correctly set

const skygearCloud = require('skygear/cloud');
const container = new skygearCloud.CloudCodeContainer();


const containerDuringInit = new skygearCloud.CloudCodeContainer({
  from_plugin: true
});


const containerAsUser = new skygearCloud.CloudCodeContainer({
  asUserId: 'user/uuid'
});

Looks good to you all?

@ben181231
Copy link
Contributor Author

The example usage looks good to me.

@rickmak
Copy link
Member

rickmak commented May 4, 2017

If no objection within 12 hours, let try yo implement it.

@rickmak
Copy link
Member

rickmak commented May 5, 2017

I am closing this PR and expect a new one.

@rickmak rickmak closed this May 5, 2017
@ben181231 ben181231 deleted the plugin-only-request branch August 1, 2018 03:39
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.

5 participants