refactor: add web framework plugin types and script to fetch types #621

kjin · 2017-12-05T00:56:57Z

Fixes #620

This change adds TypeScript types for all web framework tracing plugins and subjects them to gts, In addition, it introduces an npm verb to download type definitions separately into a directory distinct from node_modules. The purpose of this is to depend on several versions of type definitions, something that isn't supported by npm. Currently, this doesn't apply to any type definitions (Koa 1 and 2 have different types, but @types/koa@1 doesn't exist, and the minimal type definitions for koa 1 can share types with koa 2).

I'm considering avoiding this entirely (it adds a considerable amount of complexity to the codebase), and having the Trace Agent simply depend on one version of each module instead. So far this is feasible, but it might not be in the long term. I would like to get thoughts on this.

This change also fixes a bug where framework-specific routing trace labels aren't generated because the Trace Agent expects them on the wrong property. This is apparent in koa (#620) and connect (because AFAIK connect's route path is the original URL).

Update: Changed this so that plugin types are simply installed under devDependencies. There is no script to fetch types anymore. I might add that later, thinking of crossing that bridge when the times comes (currently, besides Koa 1 which has no @types module anyway, there are no modules whose interfaces change dramatically between versions)

codecov · 2018-01-19T19:56:48Z

Codecov Report

Merging #621 into master will decrease coverage by 0.41%.
The diff coverage is 90.69%.

@@            Coverage Diff             @@
##           master     #621      +/-   ##
==========================================
- Coverage      91%   90.59%   -0.42%     
==========================================
  Files          30       30              
  Lines        1390     1403      +13     
  Branches      275      288      +13     
==========================================
+ Hits         1265     1271       +6     
- Misses         53       54       +1     
- Partials       72       78       +6

Impacted Files	Coverage Δ
src/plugins/plugin-express.ts	`100% <100%> (ø)`	⬆️
src/plugins/plugin-restify.ts	`96.96% <100%> (ø)`	⬆️
src/plugins/plugin-koa.ts	`87.93% <86.11%> (-4.67%)`	⬇️
src/plugins/plugin-hapi.ts	`91.3% <87.09%> (-3.94%)`	⬇️
src/plugins/plugin-connect.ts	`91.66% <87.5%> (-2.09%)`	⬇️
src/trace-writer.ts	`85.27% <0%> (-0.78%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a212d70...1393994. Read the comment docs.

kjin · 2018-01-19T20:46:53Z

CI failures are due to flakes. In anticipation of pending reviews I'm not restarting them right now

jinwoo

Mostly nits

scripts/index.ts

@@ -2,6 +2,7 @@ const [bin, script, ...steps] = process.argv;

 import { checkInstall } from './check-install';
 import { encryptCredentials, decryptCredentials } from './credentials';
+import { getPluginTypes } from './get-plugin-types';


src/plugins/plugin-connect.ts

+
+const SUPPORTED_VERSIONS = '3.x';
+
+function getFirstHeader(req: IncomingMessage, key: string) {


src/plugins/plugin-connect.ts

@@ -42,8 +66,8 @@ function createMiddleware(api) {
      api.wrapEmitter(req);
      api.wrapEmitter(res);

-      var url = (req.headers['X-Forwarded-Proto'] || 'http') +
-        '://' + req.headers.host + req.originalUrl;
+      const url = (req.headers['X-Forwarded-Proto'] || 'http') + '://' +


src/plugins/plugin-express.ts

@@ -55,16 +58,16 @@ function patchModuleRoot(express, api) {
      api.wrapEmitter(req);
      api.wrapEmitter(res);

-      var url = req.protocol + '://' + req.hostname + req.originalUrl;
+      const url = req.protocol + '://' + req.hostname + req.originalUrl;


src/plugins/plugin-hapi.ts

+
+const SUPPORTED_VERSIONS = '8 - 16';
+
+function getFirstHeader(req: IncomingMessage, key: string) {


src/plugins/plugin-hapi.ts

-      var url = (req.headers['X-Forwarded-Proto'] || 'http') +
-      '://' + req.headers.host + req.url;
-
+      const url = (req.headers['X-Forwarded-Proto'] || 'http') + '://' +


src/plugins/plugin-koa.ts

+interface KoaModule<T> {
+  // TypeScript isn't expressive enough, but KoaModule#use should return `this`.
+  // tslint:disable-next-line:no-any
+  readonly prototype: {use: (m: T) => any};


src/plugins/plugin-koa.ts

+// propagateContext flag. The type of "next" differs between Koa 1 and 2.
+type GetNextFn<T> = (propagateContext: boolean) => T;
+
+function getFirstHeader(req: IncomingMessage, key: string) {


src/plugins/plugin-koa.ts

    }

    api.wrapEmitter(req);
    api.wrapEmitter(res);

-    const url = (req.headers['X-Forwarded-Proto'] || 'http') +
-      '://' + req.headers.host + req.url;
+    const url = (req.headers['X-Forwarded-Proto'] || 'http') + '://' +


src/plugins/plugin-koa.ts

-
+function createMiddleware(api: PluginTypes.TraceAgent): koa_1.Middleware {
+  // Koa 1 type definitions use any here.
+  // tslint:disable-next-line:no-any


…ypes

kjin · 2018-01-20T01:46:58Z

New changes are in 1393994 only

kjin · 2018-01-23T00:20:34Z

Ignoring codecov for now... will be fixed in follow-up PR.

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 5, 2017

kjin force-pushed the plugin-types branch 4 times, most recently from f10d0a5 to 1335a4f Compare December 5, 2017 01:23

kjin mentioned this pull request Dec 5, 2017

gts check/fix uses a list of files created by tslint google/gts#98

Closed

kjin force-pushed the plugin-types branch 3 times, most recently from d5f73f7 to 7f25b90 Compare December 5, 2017 22:11

kjin force-pushed the plugin-types branch from a7afb58 to e7323a4 Compare January 19, 2018 19:43

kjin requested review from DominicKramer, ofrobots and jinwoo January 19, 2018 20:46

jinwoo reviewed Jan 19, 2018

View reviewed changes

kjin force-pushed the plugin-types branch 2 times, most recently from f98e5c8 to 544bbb7 Compare January 19, 2018 22:13

kjin added 5 commits January 19, 2018 17:46

refactor: add web framework plugin types and script to fetch plugin t…

64d7338

…ypes

ci: fix ci failure

a2ba7cc

fix koa plugin in node 4 and express function shenanigans

004ed70

more conservative approach to plugin types

b04a4fa

address comments

1393994

kjin force-pushed the plugin-types branch from 00894d8 to 1393994 Compare January 20, 2018 01:46

jinwoo approved these changes Jan 20, 2018

View reviewed changes

kjin merged commit 54dd734 into googleapis:master Jan 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: add web framework plugin types and script to fetch types #621

refactor: add web framework plugin types and script to fetch types #621

kjin commented Dec 5, 2017 •

edited

Loading

codecov bot commented Jan 19, 2018 •

edited

Loading

kjin commented Jan 19, 2018

jinwoo left a comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

kjin commented Jan 20, 2018

kjin commented Jan 23, 2018


		const SUPPORTED_VERSIONS = '3.x';

		function getFirstHeader(req: IncomingMessage, key: string) {


		const SUPPORTED_VERSIONS = '8 - 16';

		function getFirstHeader(req: IncomingMessage, key: string) {

refactor: add web framework plugin types and script to fetch types #621

refactor: add web framework plugin types and script to fetch types #621

Conversation

kjin commented Dec 5, 2017 • edited Loading

codecov bot commented Jan 19, 2018 • edited Loading

Codecov Report

kjin commented Jan 19, 2018

jinwoo left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

kjin commented Jan 20, 2018

kjin commented Jan 23, 2018

kjin commented Dec 5, 2017 •

edited

Loading

codecov bot commented Jan 19, 2018 •

edited

Loading