opentracing / opentracing-javascript

OpenTracing API for Javascript (both Node and browser). 🛑 This library is DEPRECATED! https://github.com/opentracing/specification/issues/163
http://opentracing.io
Apache License 2.0
1.09k stars 108 forks source link

Update API compatibility testing documentation #78

Closed rochdev closed 7 years ago

rochdev commented 7 years ago

The current documentation says to use this snippet to run API compatibility tests:

const apiCompatibilityChecks = require('opentracing/test/api_compatibility.js');
apiCompatibilityCheck(() => new CustomTracer());

However, since version 0.14, it should be instead:

const apiCompatibilityChecks = require('opentracing/lib/test/api_compatibility.js').default;
apiCompatibilityCheck(() => new CustomTracer());

See: https://github.com/opentracing/opentracing-javascript#api-compatibility-testing

rochdev commented 7 years ago

@yurishkuro Still missing .default since TypeScript doesn't export the default directly.

yurishkuro commented 7 years ago

hm, it worked for me, although bombed on another issue #79, which might be related.

Is there a way to make TypeScript export the default?

cc @felixfbecker

rochdev commented 7 years ago

Not sure, I know it's also the new behaviour of Babel since version 6. For babel there is a plugin to get the old behaviour back, I don't know about TypeScript. This is related to converting import statements to require, so a quick and easy solution for node is to simply use require directly.

rochdev commented 7 years ago

Found a few discussions about this:

https://github.com/Microsoft/TypeScript/issues/2719 https://github.com/Microsoft/TypeScript/issues/3337

rochdev commented 7 years ago

I don't see any problems with requiring .default as long as it's documented.

felixfbecker commented 7 years ago

Yeah, I overlooked this in my PR.

yurishkuro commented 7 years ago

Given that README uses const apiCompatibilityChecks = ... (ES6 feature), wouldn't the example be better with import instead of require?

import apiCompatibilityChecks from 'opentracing/lib/test/api_compatibility.js';
felixfbecker commented 7 years ago

I would go after what the NodeJS LTS versions we are targeting support, and they don't support import yet (they do support const, arrow functions, etc). Could be confusing for users if the examples don't run without a transpiler.

.default has actually been part of the CommonJS spec before NodeJS existed so it's not strictly an ES6 concept :)

bhs commented 7 years ago

@felixfbecker @yurishkuro I'm taking a look at open issues... what is the status here? I am not knowledgeable enough about Node to have an opinion, but would like to agree on what we think is best (even if we don't have time to do the work right now).

felixfbecker commented 7 years ago

I think this should be fixed in the README on latest master:

const { apiCompatibilityChecks } = require('opentracing/lib/test/api_compatibility.js');
apiCompatibilityChecks(() => new CustomTracer());

(it destructures all the exports and cherry-picks apiComatibilityChecks)

bhs commented 7 years ago

@felixfbecker SGTM!

MarckK commented 7 years ago

See current master (Fix how requiring apiCompatibilityChecks #85 now merged in) :) The default export in opentracing/lib/test/api_compatibility.js. is a property on the exports object and thus is accessed with .default. There can be only one default property on the exports object. This property does not therefore need to be destructured.

pavolloffay commented 7 years ago

closing per #85