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

Request: separate package with only TS interfaces and enums #3

Closed
draffensperger opened this issue May 15, 2019 · 11 comments
Closed

Request: separate package with only TS interfaces and enums #3

draffensperger opened this issue May 15, 2019 · 11 comments

Comments

@draffensperger
Copy link
Contributor

I've been working on the OpenCensus Web project, which uses types from OpenCensus Node. However, one of the challenges we ran into is that the @opencensus/core NPM package that had the core trace/stats model types also imported Node-specific libraries like continuation-local-storage, etc. That made it hard to pull in the types only to make a web-specific implementation of them (I ended up writing a copy script).

I think having a clean package with no dependencies besides TypeScript and maybe proto files would be a nice way to make the Node and Web implementations share an API but be free to diverge in the specifics.

We could also try to have a shared core that is common between Node and Web environments, but even simple things like getting a high-resolution timestamp or generating a secure random number are done differently in Node vs. the browser. There are also different tradeoffs like Node wanting raw performance vs. the browser wanting small code size. So I think separate implementations is OK, but we should share common types.

@mayurkale22
Copy link
Member

+1 and If possible we can use DefinitelyTyped to host OpenTelemetry related type definition files. Might be once library is stable / production ready.

@draffensperger
Copy link
Contributor Author

Interesting - my impression was that DefinitelyTyped is primarily used for libraries written in vanilla JS but that expose additional type files. What would be the advantage of that over having a dedicated types package as part of the library?

A major advantage I'd see of a package (say @opentelemetry/types or similar) would be that we could modify it as part of the same monorepo as other packages, whereas hosting it on DefinitelyTyped would require syncing different versions back and forth. (I'm guessing that even once it's stable/production ready we will still want to have intra-release dev velocity to gradually modify the types).

@rochdev
Copy link
Member

rochdev commented May 23, 2019

DefinitelyTyped is mostly used when the module itself doesn't include the definition files, or when there are cases where you want the definition files only without pulling the library. In general it's better to include the definition files directly in the module, or as proposed above in a separate module with just interfaces.

I'm not a fan of using TypeScript for this type of project however because there are very specific JavaScript constructs that must be tested in some case, and transpilation makes this very difficult. For example, I was trying to test async/await in opentracing-javascript and since the project uses a target of es3 it would be replaced by callbacks so it was not possible to test. In some cases it's also not possible to properly optimize since TypeScript might modify the output.

I would have a preference towards pure JavaScript with definition files only. I'm no TypeScript expert however so maybe there are ways to handle the above points properly with TypeScript that I don't know about.

@draffensperger
Copy link
Contributor Author

draffensperger commented May 23, 2019

These are some interesting points - we should discuss this more at the community meeting for this. I generally favor TypeScript since its allows for catching a lot more bugs at compile time vs. run time and has nice editor features.

Regarding testing async/await that would be replaced by callbacks, would it have been possible to test it by executing the full build and then doing something like an integration test where you bring in the compiled JS and exercise that directly? I have been meaning to set up a protractor integration test for opencensus-web at some point that would exercise the full webpack build and then test that the spans that result are correct. Having at least one such integration test seems like a useful way to test the raw source + compilation process in a robust way.

Regarding optimization, at least for the browser case, I actually think TypeScript will have stronger optimization potential than raw JavaScript. That's because if we use tsickle we can generate Closure-typed JS from the TypeScript code and then feed that into the Closure compiler with its advanced optimization flags turned on, which then allows it to do things like function inlining, property renaming and dead code elimination - all resulting in smaller JS bundle sizes. Such a build can be used with webpack, e.g. in this example. What did you mean by optimization? Were you thinking of JS bundle size for the browser or more optimizing CPU/memory in Node?

@vmarchaud
Copy link
Member

For example, I was trying to test async/await in opentracing-javascript and since the project uses a target of es3 it would be replaced by callbacks so it was not possible to test

In this regard, what is the target version of both the node and browser implementation ? I believe a lot of choice will result of that.
For example, the opencensus-node only support node 8 and newer, so we could target es6 as an output for node however it will be a problem if we look for some code sharing with the open-telemetry browser, i believe typescript will not like it.

@draffensperger
Copy link
Contributor Author

From what I can tell, it's kind of tricky to write packages that work for both Node and browser (e.g. this article about it that highlights some challenges). Even seemingly simple things can differ between Node and browser, e.g. getting the high-performance time (hrtime in Node, performance.now in browser), or generating a secure random number (crypto.randomFillSync in Node, window.crypto.getRandomValues or polyfill inbrowser).

Having different implementations with shared types would allow the Node version to target say Node 8+, but still allow the browser version to target older browsers (IE 11, etc.)

I'd be curious though to hear from @rochdev or others who have worked on opentracing-javascript with how their experiences of targeting both Node and browser in the same code base felt.

@rochdev
Copy link
Member

rochdev commented May 23, 2019

I'd be curious though to hear from @rochdev or others who have worked on opentracing-javascript with how their experiences of targeting both Node and browser in the same code base felt.

Disclaimer: we don't support the browser at this time.

What we did is write the tracer in a way that is independent of the platform it runs on. Then we have a ./platform folder which contains a node and (eventually) a browser folder and they have different implementations for different things. For example, platform.now() would use process.hrtime() in Node and performance.now() in the browser (or Date if nothing else is available). In the long run, we'll probably split these with Lerna instead of simply folders.

The idea is that the tracer behaves exactly the same, so having 2 different tracer packages would result in a lot of duplication. So instead, we went with a single tracer and multiple platforms.

I think eventually that such a project could even become its own separate project that provides a common API in front of the most popular Node/browser APIs. Kind of a backward-compatible JavaScript standard library if you will.

For example, the opencensus-node only support node 8 and newer, so we could target es6 as an output for node however it will be a problem if we look for some code sharing with the open-telemetry browser, i believe typescript will not like it.

What is the target Node version for OpenTelemetry? I feel that a minimum version of Node 8 would prevent most APM vendors from using OpenTelemetry, rendering the project useless for the most part. We have customers on Node 4 and Node 6, and when I tried to propose removing support for 0.10 and 0.12 from OpenTracing it was rejected since several users are still using these versions.

@rochdev
Copy link
Member

rochdev commented May 23, 2019

I generally favor TypeScript since its allows for catching a lot more bugs at compile time vs. run time and has nice editor features.

The nice editor features in my opinion is the only feature of TypeScript. It's been demonstrated many times that type safety has nearly no benefit for a properly tested application. In general, the opposite ends up happening: reliance on types increases and the number of tests decreases along with code coverage.

For a library, this is made even worse because there is no way to control if the user of the library is using JavaScript or TypeScript.

For example, let's take the following scenario:

function send(num: Number): void {
    agent.write(JSON.stringify({ num }));
}

If you rely on TypeScript to validate the Number and the user is using JavaScript and passes a string, it's possible that this will break when it reaches the agent. There should be a test for this case, and it should be handled in code.

Of course this is not mutually exclusive with using TypeScript, but in my opinion, for a library, and especially a small one like OpenTelemetry, there are a lot more disadvantages than advantages.

Let me do a quick summary of the advantages and disadvantages in my opinion:

+ Auto-completion and other quality of life features from the editor.
+ The intent is more explicit when reading code.
- False sense of safety.
- More verbose to read and write. This is made even worse if the tests are in TypeScript as well.
- Some common JavaScript patterns are difficult to type (optional arguments, functional programming, etc)
- A build process and additional tooling is necessary for Node which usually doesn't need it.
- The behavior of the compiler can be unexpected, for example hoisting of imports.
- Difficult to test some language features depending on the target. As you mentioned, this can be addressed by adjusting the tooling, but it's difficult to get right.
- Not testing reality if the target of the tests must be different than the target of the code.

I feel like the advantages are usually more beneficial for very large projects and detrimental to smaller projects like microservices or libraries.

My main point however is that the project should be easy to work with and allow TDD, which has not been the case for every single TypeScript project I've ever worked on. If TypeScript is decided as the language of choice by the majority, I would really like to be part of the discussion to structure the project so I can make sure it's not going in a direction that will cause the many issues I'm used to see with TypeScript projects.

@rochdev
Copy link
Member

rochdev commented May 23, 2019

Regarding optimization, at least for the browser case, I actually think TypeScript will have stronger optimization potential than raw JavaScript.

Browsers always need tooling to bundle/minify/etc anyway, so I agree in this case that a few of the downsides I mentioned above are required regardless of using TypeScript. However, since JavaScript code is valid TypeScript code, isn't it possible to use tsickle on plain JavaScript, similar to the capability of the TypeScript compiler?

@draffensperger
Copy link
Contributor Author

To respond to the question on the ability of the Closure compiler to optimize plain JS: yes, it can do that too, but it has less ability to optimize, since it uses types for some optimizations. I don't have numbers on how large the effect is, but that extra types-based optimization is a major motivation for the tsickle project.

@mayurkale22
Copy link
Member

Closing this, we have already decided/implemented the separate package with only TS interfaces and enums.

Annosha referenced this issue in Annosha/opentelemetry-js Oct 19, 2024
# This is the 1st commit message:

add type declaration for test-none-core-module

# This is the commit message #2:

Replaced cpx2 with a local module

# This is the commit message #3:

fixed lint errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants