econify / graphql-rest-router Goto Github PK

While an implementation of a working in-memory cache exists, looking for cache through the code base it seems that an actual integration is still TBD.

It seems that this would be a good starting place to cache result returned by makeRequest:

Also cacheEngine and defaultCacheTimeInMs needs to be passed into Route here:

Is your feature request related to a problem? Please describe.
TypeScript best practices not followed in OpenApi.ts, using explicit any

Describe the solution you'd like
Remove explicit any's in OpenApi.ts for better TypeScript support

It seems like InMemoryCache is not exported in index.ts.

I can see two ways how this can be resolved:

1. Update README.md

- import GraphQLRestRouter, { InMemoryCache } from 'graphql-rest-router';
+ import InMemoryCache from 'graphql-rest-router/InMemoryCache';

or for CJS

const { default: InMemoryCache } = require('graphql-rest-router/InMemoryCache');

2. Export InMemoryCache in index.ts:

import Router from './Router';
import Route from './Route';
import InMemoryCache from './InMemoryCache';

import * as OpenApi from './OpenApi';
import ApiBlueprint from './ApiBlueprint';

export default Router;
export { Route, OpenApi, ApiBlueprint, InMemoryCache };

Running npm audit currently shows:

# npm audit report

axios  <0.21.1
Severity: high
Server-Side Request Forgery - https://npmjs.com/advisories/1594
fix available via `npm audit fix --force`
Will install undefined@undefined, which is a breaking change
node_modules/graphql-rest-router/node_modules/axios
  graphql-rest-router  *
  Depends on vulnerable versions of axios
  node_modules/graphql-rest-router

See https://rollupjs.org/ to get started.

Also, this warning when used with TS:

If you are using TypeScript, we recommend you explicitly list the @types packages you want to use using the types property in the "tsconfig.json" file, or set it to []. Rollup has a dependency on @types/node, which means (without this change) these types will automatically be available in your app even when some of them should not be available based on the target you are using.

There's currently 4 failing tests that need to be fixed.

Here's the output from running npm run test:

1. Route
   #constructor
   with valid arguments
   when using minimal configuration
   should set path to the operation name:

   AssertionError: expected '/GetUserById' to equal 'GetUserById'

   • expected - actual

   -/GetUserById
   +GetUserById

   at Context. (test/Route.test.js:20:18)
   at processImmediate (internal/timers.js:456:21)

2. Route
   private#setOperationName
   when given an operation name that exists in the schema
   should set operation variables:
   AssertionError: expected { Object (id) } to deeply equal [ Array(1) ]
   at Context. (test/Route.test.js:114:16)
   at processImmediate (internal/timers.js:456:21)

3. Route
   #path
   should use the operation name as the default path:

   AssertionError: expected '/GetUserById' to equal 'GetUserById'

   • expected - actual

   -/GetUserById
   +GetUserById

   at Context. (test/Route.test.js:124:14)
   at processImmediate (internal/timers.js:456:21)

4. Router
   #mount
   argument overloading
   should get operation name from configuration if only single argument provided:
   TypeError: operationNameOrMountableItem.withOptions is not a function
   at Router.mount (Router.js:80:58)
   at Context. (test/Router.test.js:67:16)
   at processImmediate (internal/timers.js:456:21)

I need to make sure that I understand the proper passing tests before tackling this one.

Module currently uses decoded path variables via req params:

asExpressRoute() {
    return async (req: express.Request, res: express.Response) => {
      const { query, params, body } = req;
      
      const parsedQueryVariables = this.typecastVariables(query);
      const parsedPathVariables = this.typecastVariables(params);
...

https://github.com/Econify/graphql-rest-router/blob/master/Route.ts#L290

req.params implicitly decodes each part of the path into a corresponding path variable.

If one of the path variables is used as a slug, then that slug would need to be stored decoded in the database in order to be looked up. Storing a slug as a decoded path means the slug is no longer a valid part of a URL. If the slug is used for embedded URLs this means it will have to be encoded every time.

Since a slug is part of URL, believe it is a reasonable expectation of consumers of the library to store slugs in a valid encoded form.

To my knowledge, because const { params } = req; implicitly decodes each value in the object, it is currently impossible to look up an encoded slug.

Thoughts/ideas/gotchas?

Describe the solution you'd like
Complete the API Blueprint implementation found in ApiBlueprint.ts

Describe the solution you'd like
Add autoDiscoverEndpoints to global config. Description removed from readme on promotion to beta-0 ( see text below).

| autoDiscoverEndpoints | boolean | false | When set to true, GQL Rest Router will scan the provided client schema you provide and automatically mount an endpoint for each operation name / named query |

Not passing passThroughHeaders into mount(..., { passThroughHeaders: [] }) results in the following error:

node_modules/graphql-rest-router/Router.js:70
                ? [...this.passThroughHeaders, ...options.passThroughHeaders]
                                                          ^
TypeError: options.passThroughHeaders is not iterable
    at Router.mount (/Users/tw/Projects/curator-stack/api/node_modules/graphql-rest-router/Router.js:70:59)
    at makeGQLRestRouter (/Users/tw/Projects/curator-stack/api/tests/load-test.js:37:6)

Changing

-      const passThroughHeaders = Boolean(options)
+      const passThroughHeaders = Boolean(options) && Boolean(options.passThroughHeaders)

here

should fix the issue.