Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Update API compatibility testing documentation #78

Closed
rochdev opened this issue Apr 24, 2017 · 13 comments
Closed

Update API compatibility testing documentation #78

rochdev opened this issue Apr 24, 2017 · 13 comments

Comments

@rochdev
Copy link
Contributor

rochdev commented Apr 24, 2017

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
Copy link
Contributor Author

rochdev commented Apr 24, 2017

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

@yurishkuro
Copy link
Member

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

@yurishkuro yurishkuro reopened this Apr 24, 2017
@rochdev
Copy link
Contributor Author

rochdev commented Apr 24, 2017

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
Copy link
Contributor Author

rochdev commented Apr 24, 2017

Found a few discussions about this:

microsoft/TypeScript#2719
microsoft/TypeScript#3337

@rochdev
Copy link
Contributor Author

rochdev commented Apr 24, 2017

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

@felixfbecker
Copy link
Contributor

Yeah, I overlooked this in my PR.

@yurishkuro
Copy link
Member

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
Copy link
Contributor

felixfbecker commented Apr 25, 2017

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
Copy link
Contributor

bhs commented May 27, 2017

@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
Copy link
Contributor

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
Copy link
Contributor

bhs commented May 28, 2017

@felixfbecker SGTM!

@MarckK
Copy link
Contributor

MarckK commented Oct 30, 2017

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
Copy link
Member

closing per #85

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants