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 environment to plugin #443

Merged
merged 3 commits into from Aug 21, 2017
Merged

Add environment to plugin #443

merged 3 commits into from Aug 21, 2017

Conversation

Xuanwo
Copy link
Contributor

@Xuanwo Xuanwo commented Aug 17, 2017

As discussed in slack: https://getinsomnia.slack.com/archives/C4Z8M9E86/p1502912037000177, this pr intended to add these features:

  • Allow plugin to access current context, so plugin's can get environment data now.
  • Add getAllHeaders API for context request

With these improves:

  • Use JSON to parse npm show's output, it's indeed a JSON content, we don't need to parse it with split in \n.

Copy link
Contributor

@gschier gschier left a comment

Choose a reason for hiding this comment

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

A few minor comments 😄

@@ -661,13 +662,13 @@ export async function send (requestId: string, environmentId: string) {
return _actuallySend(renderedRequest, workspace, settings);
}

async function _applyRequestPluginHooks (renderedRequest: RenderedRequest): Promise<RenderedRequest> {
async function _applyRequestPluginHooks (renderedRequest: RenderedRequest, renderdContext: Object = {}): Promise<RenderedRequest> {
Copy link
Contributor

Choose a reason for hiding this comment

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

The default value = {} for renderdContext is not needed here since it is a required parameter. Also, it should be spelled renderedContext 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got, I will fix it.

@@ -3,7 +3,7 @@ import type {Plugin} from '../';
import * as electron from 'electron';
import {showAlert} from '../../ui/components/modals/index';

export function init (plugin: Plugin): {app: Object} {
export function init (plugin: Plugin, renderdContext: Object = {}): {app: Object} {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would make more sense for the renderedContext to be part of plugins/context/request.js instead of here. The renderedContext parameter should also be required (no default value).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only for request ? Maybe template tags also need this ability.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are correct that environments might be useful for more than just request hooks. It may also be useful to have in the response hooks as well.

The main reason that I don't want it in app is that app is meant for application-level actions (eg. getting user input) so environment functionality doesn't fit very well.

I would be open to adding a context/environment.js that is exposed in both request and response hooks.

Copy link
Contributor

Choose a reason for hiding this comment

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

And maybe later I can also fix the template tags plugin API to use this as well 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will add it in request instead of app.

@@ -17,6 +17,12 @@ export function init (plugin: Plugin): {app: Object} {
throw new Error(`Unknown path name ${name}`);
}
},
getEnv (name: string): string | Object {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rename these to getEnvironmentVariable and getEnvironment? Also, the return type should be any type that can be represented in JSON string | number | boolean | Object | Array<any> | null | undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I will do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

undefined looks like can't be used in type annotation, I faced this error

app/plugins/context/request.js:77
 77:       getEnvironment (): string | number | boolean | Object | Array<any> | null | undefined {
                                                                                       ^^^^^^^^^ undefined. Ineligible value used in/as type annotation (did you forget 'typeof'?)
 77:       getEnvironment (): string | number | boolean | Object | Array<any> | null | undefined {
                                                                                       ^^^^^^^^^ undefined

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, you might have to use the question mark instead.

?(string | number | boolean | Object | Array<any> | null)

@@ -17,6 +17,12 @@ export function init (plugin: Plugin): {app: Object} {
throw new Error(`Unknown path name ${name}`);
}
},
getEnv (name: string): string | Object {
return (renderdContext && renderdContext[name]) || '';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine just returning renderedContext[name] here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, I will do it.

@@ -33,6 +33,9 @@ export function init (plugin: Plugin, renderedRequest: RenderedRequest): {reques
return null;
}
},
getHeaders (): Array<RequestHeader>|null {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the RequestHeader type has some extra field in it that we don't want to share with the user, this could be changed to just return name and value:

getHeaders (): Array<{name: string, value: string}> {
  return renderedRequest.headers.map(h => ({ name: h.name, value: h.value })); 
},

This also ensures that we don't accidentally expose any new fields to plugins that are added internally.

Also, the null type is not necessary because this will still return an empty array if there are no headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, and I made a mistake that this API should be getAllHeaders, what do you think about that?

// Strip quotes off of the value
info[match[1]] = match[2];
}
const info = JSON.parse(stdout);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work? npm show seemed to return JS objects, not JSON.

image

Copy link
Contributor Author

@Xuanwo Xuanwo Aug 17, 2017

Choose a reason for hiding this comment

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

But the old way can't handle these:

insomnia: 
   { name: 'insomnia-qingstor',
     description: 'QingStor signature plugin for insomnia.' },

maybe we should find a better way.

Copy link
Contributor

@gschier gschier Aug 17, 2017

Choose a reason for hiding this comment

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

Oh yes, you are correct.

After some searching, I found that you can specify --json flag on the npm show to get JSON output. That should work with your code 👍

https://docs.npmjs.com/cli/view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I just need to add --json for the commands, great, I wiil do it.

@Xuanwo
Copy link
Contributor Author

Xuanwo commented Aug 17, 2017

@gschier All changes have been done except the npm show's one.

Copy link
Contributor

@gschier gschier left a comment

Choose a reason for hiding this comment

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

Nice, almost there 😄

expect(Object.keys(result)).toEqual(['request']);
expect(Object.keys(result.request)).toEqual([
'getId',
'getName',
'getUrl',
'getMethod',
'getHeader',
'getAllHeaders',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make sure the new methods you added are tested in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will do it.

@@ -33,6 +38,9 @@ export function init (plugin: Plugin, renderedRequest: RenderedRequest): {reques
return null;
}
},
getAllHeaders (): Array<RequestHeader> | null {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I like name getHeaders like you had before, and I think you forgot to address this comment: #443 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, you are right, I wiil fix it.

Signed-off-by: Xuanwo <xuanwo.cn@gmail.com>
Signed-off-by: Xuanwo <xuanwo.cn@gmail.com>
Signed-off-by: Xuanwo <xuanwo.cn@gmail.com>
@Xuanwo
Copy link
Contributor Author

Xuanwo commented Aug 18, 2017

Is everything ready to go?

@gschier
Copy link
Contributor

gschier commented Aug 18, 2017

Just heading to bed. I will take another look tomorrow.

@Xuanwo
Copy link
Contributor Author

Xuanwo commented Aug 18, 2017

Wow, it's an another new day for me.

😃 , see you tomorrow (for your timezone).

Copy link
Contributor

@gschier gschier left a comment

Choose a reason for hiding this comment

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

Awesome, looks good now @Xuanwo! I'll make sure it gets into the next release 😄

@gschier gschier merged commit e805d86 into Kong:develop Aug 21, 2017
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.

None yet

2 participants