zipkin-javascript-opentracing icon indicating copy to clipboard operation
zipkin-javascript-opentracing copied to clipboard

Warnings with require statement

Open laurentgilly opened this issue 7 years ago • 6 comments

Thanks for the great work on this repo. Currently I use typescript, webpack and when using the lib it generates the two following warning.

WARNING in ./node_modules/zipkin/lib/InetAddress.js 62:23-30 Critical dependency: require function is used in a way in which dependencies cannot be statically extracted @ ./node_modules/zipkin/lib/index.js @ ./node_modules/zipkin-javascript-opentracing/lib/index.js @ ./src/index.tsx

WARNING in ./node_modules/zipkin-transport-http/lib/HttpLogger.js 17:27-34 Critical dependency: require function is used in a way in which dependencies cannot be statically extracted @ ./node_modules/zipkin-transport-http/lib/index.js @ ./node_modules/zipkin-javascript-opentracing/lib/index.js @ ./src/index.tsx

Everything seems working fine though, traces are sent to zipkin and no crash when running the app. Because there is no .d.ts contains I neeeded to do const ZipkinOpentracing = require('zipkin-javascript-opentracing')instead of import ZipkinOpentracing from 'zipkin-javascript-opentracing'. Not an expert and don't know if it will resolve the issue and make the warnings dissaper but I read a lot about webpack and errors with require().

laurentgilly avatar Jul 12 '18 13:07 laurentgilly

Could you try import * as ZipkinOpentracing from 'zipkin-javascript-opentracing'? It's just a lucky guess

DanielMSchmidt avatar Jul 12 '18 14:07 DanielMSchmidt

Yep already tried but same error regarding typescript: Could not find a declaration file for module 'zipkin-javascript-opentracing'

laurentgilly avatar Jul 12 '18 15:07 laurentgilly

But that is correct, we have no typescript definitions so far. Would be super awesome to get some, would love to see a PR :)

DanielMSchmidt avatar Jul 12 '18 15:07 DanielMSchmidt

Here you can look at my PR for the project Definitely Typescript: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/27251 It will give the possibility to npm install @types/zipkin-javascript-opentracing and use nice import without "require". Currently there is an issue on the build because another package (typescript version). If it takes too many times I will directly added the definition inside this repo and submit to you a PR.

Hope it will help when it's merge ;-)

laurentgilly avatar Jul 13 '18 09:07 laurentgilly

-1 for external typings. The best solution is to convert the module to typescript. What do you think about it @DanielMSchmidt ?

The second one is to provide build-in typings. I've improved typings from @laurentgilly . They are still not perfect, but more precise:

	import {
		FORMAT_BINARY,
		FORMAT_TEXT_MAP,
		FORMAT_HTTP_HEADERS,
		Span,
		SpanOptions
	} from 'opentracing';
	import { Recorder, sampler, Tracer } from 'zipkin';

	// at least one of them should be set
	type RecorderEndpoint = {
		recorder: Recorder;
	} | {
		endpoint: string;
	} | {
		recorder: Recorder;
		endpoint: string;
	}

	interface Options {
		serviceName: string;
		kind: 'client' | 'server' | 'local';
		sampler?: sampler.Sampler;
	}

	type TracingOptions = Options & RecorderEndpoint;

	class Tracing {
		constructor(options: TracingOptions);

		extract(format: typeof FORMAT_HTTP_HEADERS, carrier: any): Span;

		inject(span: Span, format: typeof FORMAT_HTTP_HEADERS, carrier: any): void;

		startSpan(name: string, options: SpanOptions): Span;

		static FORMAT_BINARY: typeof FORMAT_BINARY;

		static FORMAT_HTTP_HEADERS: typeof FORMAT_HTTP_HEADERS;

		static FORMAT_TEXT_MAP: typeof FORMAT_TEXT_MAP;

		static makeOptional(val: any): any;
	}

	export default Tracing

Bessonov avatar Aug 05 '19 15:08 Bessonov

Moving to TypeScript would be really cool, would happily review that PR :)

DanielMSchmidt avatar Aug 08 '19 07:08 DanielMSchmidt