Add environment to plugin #443

Xuanwo · 2017-08-17T16:07:37Z

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.

gschier

A few minor comments 😄

gschier · 2017-08-17T16:15:38Z

app/network/network.js

@@ -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> {


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

Got, I will fix it.

gschier · 2017-08-17T16:20:00Z

app/plugins/context/app.js

@@ -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} {


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).

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

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.

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

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

gschier · 2017-08-17T16:20:49Z

app/plugins/context/app.js

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


Yep, I will do it.

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

gschier · 2017-08-17T16:24:11Z

app/plugins/context/app.js

@@ -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]) || '';


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

Great, I will do it.

gschier · 2017-08-17T16:28:57Z

app/plugins/context/request.js

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


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.

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

gschier · 2017-08-17T16:36:00Z

app/plugins/install.js

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


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

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.

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

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

Xuanwo · 2017-08-17T17:04:27Z

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

gschier

Nice, almost there 😄

gschier · 2017-08-17T17:18:47Z

app/plugins/context/__tests__/request.test.js

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


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

Sure, I will do it.

gschier · 2017-08-17T17:20:30Z

app/plugins/context/request.js

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


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

Yep, you are right, I wiil fix it.

Signed-off-by: Xuanwo <xuanwo.cn@gmail.com>

Xuanwo · 2017-08-18T05:37:05Z

Is everything ready to go?

gschier · 2017-08-18T06:14:00Z

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

Xuanwo · 2017-08-18T06:15:52Z

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

😃 , see you tomorrow (for your timezone).

gschier

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

gschier suggested changes Aug 17, 2017

View reviewed changes

Xuanwo added 3 commits August 18, 2017 01:44

Allow request in plugin to access current context

11d2ba0

Signed-off-by: Xuanwo <xuanwo.cn@gmail.com>

Add getAllHeaders API for context request

d55f194

Signed-off-by: Xuanwo <xuanwo.cn@gmail.com>

Use JSON to parse npm show's output

10ee69d

Signed-off-by: Xuanwo <xuanwo.cn@gmail.com>

gschier approved these changes Aug 21, 2017

View reviewed changes

gschier merged commit e805d86 into Kong:develop Aug 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add environment to plugin #443

Add environment to plugin #443

Xuanwo commented Aug 17, 2017

gschier left a comment

gschier Aug 17, 2017

Xuanwo Aug 17, 2017

gschier Aug 17, 2017

Xuanwo Aug 17, 2017

gschier Aug 17, 2017

gschier Aug 17, 2017

Xuanwo Aug 17, 2017

gschier Aug 17, 2017

Xuanwo Aug 17, 2017

Xuanwo Aug 17, 2017

gschier Aug 17, 2017

gschier Aug 17, 2017

Xuanwo Aug 17, 2017

gschier Aug 17, 2017

Xuanwo Aug 17, 2017

gschier Aug 17, 2017

Xuanwo Aug 17, 2017 •

edited

Loading

gschier Aug 17, 2017 •

edited

Loading

Xuanwo Aug 17, 2017

Xuanwo commented Aug 17, 2017 •

edited

Loading

gschier left a comment

gschier Aug 17, 2017

Xuanwo Aug 17, 2017

gschier Aug 17, 2017

Xuanwo Aug 17, 2017

Xuanwo commented Aug 18, 2017

gschier commented Aug 18, 2017

Xuanwo commented Aug 18, 2017 •

edited

Loading

gschier left a comment

Add environment to plugin #443

Add environment to plugin #443

Conversation

Xuanwo commented Aug 17, 2017

gschier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xuanwo Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

gschier Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xuanwo commented Aug 17, 2017 • edited Loading

gschier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xuanwo commented Aug 18, 2017

gschier commented Aug 18, 2017

Xuanwo commented Aug 18, 2017 • edited Loading

gschier left a comment

Choose a reason for hiding this comment

Xuanwo Aug 17, 2017 •

edited

Loading

gschier Aug 17, 2017 •

edited

Loading

Xuanwo commented Aug 17, 2017 •

edited

Loading

Xuanwo commented Aug 18, 2017 •

edited

Loading