Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: transpile to es2017 as esnext may result in unsupported JS code #593

Merged

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Dec 5, 2019

Which problem is this PR solving?

Unsupported JS code is generated if e.g. optional chaining operator ?. is used.

Short description of the changes

Transpile to es2017 to ensure compatiblity with Node.Js 8. Otherwise use of
e.g. the optional chaining operator ?. supported since typescript 3.7 results
in js not running on nodejs 8. Es2017 is the minimum to get native await support.

Transpile to es2017 to ensure compatiblity with Node.Js 8. Otherwise use of
e.g. the optional chaining operator ?. supported since typescript 3.7 results
in js not running on nodejs 8. Es2017 is the minimum to get native await support.
@dyladan
Copy link
Member

dyladan commented Dec 5, 2019

Is there a reason we even need to use es2017? On my machine es2015 produces a build that is nearly the same size, taking only a couple more seconds to build. Is there any real advantage to targeting such a late version of node?

esnext => ~45 seconds, 3988k for all packages
es2015 => ~50 seconds, 3996k for all packages (0.2% larger)

@vmarchaud
Copy link
Member

@dyladan I believe that i may introduce polyfill in javascript code that may be slow and since we don't really have a benchmarking in place to mesure if we compile to slow code, i would prefer to use es2017 here

@dyladan
Copy link
Member

dyladan commented Dec 5, 2019

Maybe in the future we could use a tagged release mechanism. Install with npm install @opentelemetry/[email protected].

It is probably too much for alpha or even beta, but could be useful once we are 1.0

Copy link
Member

@OlivierAlbertini OlivierAlbertini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm agree with @vmarchaud

@Flarna
Copy link
Member Author

Flarna commented Dec 5, 2019

I think it's not just performance. Polyfills tend to have hidden corners where they act different. I would assume that polyfills result in breaking the async stack trace feature introduced in nodejs 12. I could also imagine that async hooks show slightly different results.

if we ban use of await in @opentelemetry packages transpile to older standards would be easier but I see no reason to do so.

Maybe in the future we could use a tagged release mechanism. Install with npm install @opentelemetry/[email protected].

I think this breaks semver range patterns (e.g. ^0.2.0).

Just to illustrate the difference some small typescript sample using await:

async function bar() { return Promise.resolve(15) }
async function foo() { return await bar() }

es2017:

async function bar() { return Promise.resolve(15); }
async function foo() { return await bar(); }

es2015:

var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
    function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); }
    return new (P || (P = Promise))(function (resolve, reject) {
        function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } }
        function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } }
        function step(result) { result.done ? resolve(result.value) : adopt(result.value).then(fulfilled, rejected); }
        step((generator = generator.apply(thisArg, _arguments || [])).next());
    });
};
function bar() {
    return __awaiter(this, void 0, void 0, function* () { return Promise.resolve(15); });
}
function foo() {
    return __awaiter(this, void 0, void 0, function* () { return yield bar(); });
}

es5:

var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) {
    function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); }
    return new (P || (P = Promise))(function (resolve, reject) {
        function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } }
        function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } }
        function step(result) { result.done ? resolve(result.value) : adopt(result.value).then(fulfilled, rejected); }
        step((generator = generator.apply(thisArg, _arguments || [])).next());
    });
};
var __generator = (this && this.__generator) || function (thisArg, body) {
    var _ = { label: 0, sent: function() { if (t[0] & 1) throw t[1]; return t[1]; }, trys: [], ops: [] }, f, y, t, g;
    return g = { next: verb(0), "throw": verb(1), "return": verb(2) }, typeof Symbol === "function" && (g[Symbol.iterator] = function() { return this; }), g;
    function verb(n) { return function (v) { return step([n, v]); }; }
    function step(op) {
        if (f) throw new TypeError("Generator is already executing.");
        while (_) try {
            if (f = 1, y && (t = op[0] & 2 ? y["return"] : op[0] ? y["throw"] || ((t = y["return"]) && t.call(y), 0) : y.next) && !(t = t.call(y, op[1])).done) return t;
            if (y = 0, t) op = [op[0] & 2, t.value];
            switch (op[0]) {
                case 0: case 1: t = op; break;
                case 4: _.label++; return { value: op[1], done: false };
                case 5: _.label++; y = op[1]; op = [0]; continue;
                case 7: op = _.ops.pop(); _.trys.pop(); continue;
                default:
                    if (!(t = _.trys, t = t.length > 0 && t[t.length - 1]) && (op[0] === 6 || op[0] === 2)) { _ = 0; continue; }
                    if (op[0] === 3 && (!t || (op[1] > t[0] && op[1] < t[3]))) { _.label = op[1]; break; }
                    if (op[0] === 6 && _.label < t[1]) { _.label = t[1]; t = op; break; }
                    if (t && _.label < t[2]) { _.label = t[2]; _.ops.push(op); break; }
                    if (t[2]) _.ops.pop();
                    _.trys.pop(); continue;
            }
            op = body.call(thisArg, _);
        } catch (e) { op = [6, e]; y = 0; } finally { f = t = 0; }
        if (op[0] & 5) throw op[1]; return { value: op[0] ? op[1] : void 0, done: true };
    }
};
function bar() {
    return __awaiter(this, void 0, void 0, function () { return __generator(this, function (_a) {
        return [2 /*return*/, Promise.resolve(15)];
    }); });
}
function foo() {
    return __awaiter(this, void 0, void 0, function () { return __generator(this, function (_a) {
        switch (_a.label) {
            case 0: return [4 /*yield*/, bar()];
            case 1: return [2 /*return*/, _a.sent()];
        }
    }); });
}

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@Flarna Flarna requested a review from dyladan as a code owner December 13, 2019 21:03
@codecov-io
Copy link

codecov-io commented Dec 13, 2019

Codecov Report

Merging #593 into master will decrease coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #593      +/-   ##
==========================================
- Coverage   92.31%   92.16%   -0.15%     
==========================================
  Files         177      182       +5     
  Lines        8728     9049     +321     
  Branches      758      797      +39     
==========================================
+ Hits         8057     8340     +283     
- Misses        671      709      +38
Impacted Files Coverage Δ
...ges/opentelemetry-plugin-https/test/utils/utils.ts 33.33% <0%> (-26.67%) ⬇️
...ges/opentelemetry-exporter-zipkin/src/transform.ts 100% <0%> (ø) ⬆️
.../opentelemetry-exporter-zipkin/test/zipkin.test.ts 100% <0%> (ø) ⬆️
packages/opentelemetry-web/src/WebTracer.ts 100% <0%> (ø) ⬆️
packages/opentelemetry-plugin-https/src/utils.ts 100% <0%> (ø) ⬆️
...telemetry-scope-base/test/NoopScopeManager.test.ts 100% <0%> (ø) ⬆️
...entelemetry-exporter-zipkin/test/transform.test.ts 100% <0%> (ø) ⬆️
...ackages/opentelemetry-exporter-zipkin/src/utils.ts
...res/opentelemetry-plugin-pg/test/assertionUtils.ts 100% <0%> (ø)
...postgres/opentelemetry-plugin-pg/test/testUtils.ts 15% <0%> (ø)
... and 5 more

@dyladan dyladan added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Dec 13, 2019
@mayurkale22 mayurkale22 merged commit 4b25d4a into open-telemetry:master Dec 16, 2019
@Flarna Flarna deleted the transpile-es2017 branch December 16, 2019 22:26
@Flarna Flarna mentioned this pull request Apr 6, 2020
2 tasks
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
…ry#593)

* feat: move aws/gcp detectors from opentelemetry-js repo

* fixup

* fixup!

* fixup! add useful links

* Update detectors/node/opentelemetry-resource-detector-aws/package.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants