Import problems with 0.14

[yurishkuro]

I was testing 0.14 with jaeger-client-node, and ran into the following issue. Our client is written in ES6 and transpiled to JS with babel.

https://github.com/uber/jaeger-client-node/blob/master/test/udp_sender.js

// L28
import opentracing from 'opentracing';
// L124
let childOfRef = new opentracing.Reference(opentracing.REFERENCE_CHILD_OF, childOfContext);

compiled to

var _opentracing = require('opentracing');
var _opentracing2 = _interopRequireDefault(_opentracing);
// ...
var childOfRef = new _opentracing2.default.Reference(_opentracing2.default.REFERENCE_CHILD_OF, childOfContext);

When running throws this error:

        var childOfRef = new _opentracing2.default.Reference(_opentracing2.def
                                                  ^
TypeError: Cannot read property 'Reference' of undefined

@felixfbecker - any idea?

[felixfbecker]

That import statement is wrong. It should be

import * as opentracing from 'opentracing'

[yurishkuro]

ACK, that fixes it. But isn't it a serious incompatibility with 0.13?

[felixfbecker]

@yurishkuro will respond to #78 (comment) here.

TypeScript does export the default when you have a default export:

export default class IAmTheDefault {}
export class IAmNot {}

can be imported correctly with

import IAmTheDefault from '...'
// or
import { default as IAmTheDefault, IAmNot } from '...'

to get an object of all exports, use import * as ....
It makes absolutely no sense to default the default export to the "all exports" object and Babel 5's behavior is plain wrong here. They fixed it in the newest version, so this is definitely wrong usage that happened to work before because of a Babel bug/incorrect behaviour. Since 0.14 is a breaking version over 0.13 and the fix is easy I don't think we should add a workaround for this.

cc @rochdev

[yurishkuro]

Since 0.14 is a breaking version over 0.13 and the fix is easy I don't think we should add a workaround for this.

My concern is more about the actual services using the API. I don't mind fixing jaeger-client itself to use the recommended syntax, but fixing the real services is extremely difficult. But the services are all in Node 0.10, not using Babel. I'll continue experimenting.

[felixfbecker]

Is it really? It's just a global search & replace (and doesn't break anything until you update the package)

[rochdev]

According to semantic versioning, a minor version bump over 0.x is actually a major revision and can thus contain breaking changes. However, it should be properly documented. I also don't necessarily see the point of compiling such a small library anyway.

[felixfbecker]

@rochdev I briefly mentioned it in the changelog: https://github.com/opentracing/opentracing-javascript/blob/master/CHANGELOG.rst#0140

The main entry point can no longer be imported with a default import

[yurishkuro]

The main entry point can no longer be imported with a default import

We should elaborate on that, give the workaround.

[felixfbecker]

Yeah, sorry, I thought LightStep was the only Tracer that depended on this. It wasn't used in any examples.

I would add

If you previously used a default import like this:

import opentracing from 'opentracing'

change it to

import * as opentracing from 'opentracing'

[yurishkuro]

It's just a global search & replace (and doesn't break anything until you update the package)

far from being that simple. We have 100s of services in 100s of individual Git repos with all kinds of cross-dependencies. The upgrade will happen in a low-level infra library. Fortunately, most of them do not import opentracing directly, usually indirectly, to avoid dependency hell:

var opentracing = require('jaeger-client').opentracing; 

But some do import it directly, like this:

var opentracing = require('opentracing'); 

Is that going to break after TypeScript? I can't verify this at the moment.

[felixfbecker]

No, that will give the expected result. It's equivalent to import * as opentracing

[felixfbecker]

If your services are all using ES5 then they should all continue working :)

[yurishkuro]

+1, verified.