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

How to Compare Two Definitions Structurally? #7535

Closed
blakeembrey opened this issue Mar 16, 2016 · 17 comments
Closed

How to Compare Two Definitions Structurally? #7535

blakeembrey opened this issue Mar 16, 2016 · 17 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@blakeembrey
Copy link
Contributor

I'm not sure if this feature is currently supported, or even whether the whole chain of features have been enabled to make this workflow work, but at the lowest level how would one compare two definitions (1.d.ts and 2.d.ts) and check if they're compatible? On top of this, would it be possible to access the definitions for a JavaScript library?

Reasoning: It'd be great to be able to type existing JavaScript libraries, but the biggest thing hindering third-parties is being able to integrate the workflow with CI. One way to improve this workflow would be with the --allowJs flag on the library itself to generate one definition, then comparing it to the user-written definition. Having the declarations flag work with --allowJs would also allow users to get started writing definitions 10x faster - just run tsc entry.js --allowJs --declaration --out definitions and improve the types little by little.

@basarat
Copy link
Contributor

basarat commented Mar 16, 2016

but at the lowest level how would one compare two definitions (1.d.ts and 2.d.ts) and check if they're compatible

Just compile:

import * as foo from "./foo";
import * as bar from "./bar";

let _foo: typeof foo;
let _bar: typeof bar;
_foo = _bar = _foo;

I haven't tested but should work 🌹

@blakeembrey
Copy link
Contributor Author

@basarat Interesting hack. I guess I'd have to generate this file, assuming it works, to do the test. The only outstanding piece is allowJs and declarations then.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 16, 2016

allowJs with --declaration should be easy to enable. the emitter needs to be hardened to handle missing types, something it does not expect today. we would accept a PR for this if you are interested.

The only restriction i see, there has to be a --out or --outDir to make sure that a.d.t.s is not dropped next to a.js so that next compilation will not pick the output a.d.ts (as .d.ts files are preferred to .js files).

@mhegazy
Copy link
Contributor

mhegazy commented Mar 16, 2016

@blakeembrey can you log an issue to track allowing --allowJs with --declaration?

@mhegazy
Copy link
Contributor

mhegazy commented Mar 16, 2016

As for the structural comparison, who is doing this? is it a command-line tool? do you expect it to be part of the compiler? how would it be surfaced to the user? what is the intended result?

if it is a separate tool, you can do what @basarat suggested.

@mhegazy mhegazy added the Question An issue which isn't directly actionable in code label Mar 16, 2016
@blakeembrey
Copy link
Contributor Author

can you log an issue

Absolutely.

As for the structural comparison, who is doing this?

This is for writing type definitions with package authors. Definitely a CLI tool. I don't expect it to be part of the compiler. It can be part of @typings to validate a type definition on top of a JavaScript project. The intended result is that new, breaking API changes in JS have a chance of being caught by CI. For example, implementing typings for Express.js - references: expressjs/express#2818 and typings/typings#322.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 16, 2016

Using the compiler API, we can definitely expose a new method on the TypeChecker to compare 2 types. so you would walk over the .d.ts file and the .js file simultaneously and get type for each export, and then compare their types.

@blakeembrey
Copy link
Contributor Author

@mhegazy That sounds like it'd be perfect 👍

@mhegazy
Copy link
Contributor

mhegazy commented Mar 29, 2016

Feel free to file an issue for the API changes as needed.

@mhegazy mhegazy closed this as completed Mar 29, 2016
@blakeembrey
Copy link
Contributor Author

@mhegazy I couldn't find an existing issue and it seems more realistic as this same request, but would the TypeScript compiler consider exposing a mode where it only type checks .d.ts (header files) against .js (runtime files)? That's basically the request here, but without additional files being emitted. It'd allow people to adopt .d.ts files into projects and keep them maintained and up-to-date properly, a massive win for the ecosystem.

I was linked to a new project doing something like this, which renewed my interest in having a feature like this exposed (https://github.com/Raynos/jsig2).

@mhegazy
Copy link
Contributor

mhegazy commented Apr 27, 2016

We will need a proposal for what that means to implement. also what are the scenarios you would use this mode. i would be inclined to say this is a tool that the compiler API can support. but I would like to understand the full picture first.

@blakeembrey
Copy link
Contributor Author

Anywhere we're working to merge .d.ts files into a JavaScript project. For example, moment or express. Right now, they have to guess changes to the .d.ts files. Ideally, they could write the header files and keep the JavaScript files and run tsc to catch type errors between them (like the linked project). This would guarantee that the people adopting writing definitions can keep them updated automatically (E.g. as part of CI). Without this, it'll need to be done manually and they'll need to check/remind every person that does a PR and changes the external .js shape - which is a difficult task to do manually.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 27, 2016

how far would this go, like there are JS dynamic constructs that can not be verified statically, e.g.

var mod = require("./other");
for(var i in mod) {
    exports[i + "_extension"] = mod[i];
}

how would you check this against a .d.ts file?

@blakeembrey
Copy link
Contributor Author

I don't think there's any requirement to analyse that, but could it be allowed if there's extra exports in the .d.ts file that didn't match exports in the .js file and TypeScript detects a dynamic export? That would be a requirement for things like the Express.js router.

Basically, I think it'd be amazing if we can have a JavaScript/TypeScript file and create one .d.ts file that's part of the workflow and the .d.ts (header file) is type checked against the body (the .js file) as part of compilation. Maybe a flag to do this and libraries can use --noEmit?

@mhegazy
Copy link
Contributor

mhegazy commented Apr 27, 2016

We should be able to create a .d.ts from a .js file. but i would think of this as a manual process, like you create the .d.ts to get you started. but then you would manually update the .d.ts file.
The issue with checking against a .d.ts file, is which is the truth? and how far are you willing to allow differences. and if it would still be useful after adding exceptions for dynamic content.
i think js code will be moving into more static shape at least for modules with the ES6 modules, so these dynamic cases might not be an issue in the future.

@DanielRosenwasser
Copy link
Member

You may want to take a look at TSEvolve which takes two .js files and an existing .d.ts and can point out where changes have taken place. In principle, I don't see a reason why the comparison logic there couldn't be applied in this case. @webbiesdk, I believe you're involved in that project, right?

@webbiesdk
Copy link

Yes, I made that.

Yes, it could be used.
If used in a CI setting, it could list the changes between HEAD and the latest stable release.
Since it is based on an unsound type inference, it will list spurious changes, and it can therefore only be used as a help to whoever is updating the .d.ts file.

Currently the project uses TypeScript 1.5 to parse the .d.ts files, and it doesn't work with NodeJS libraries. (Updating to TypeScript 2.1 shouldn't take long, but getting it to work with NodeJS could be quite a hassle).

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

5 participants