Skip to content

Compilation fails on Cannot find name 'T' #116

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

Closed
woss opened this issue Aug 31, 2021 · 12 comments · Fixed by #117
Closed

Compilation fails on Cannot find name 'T' #116

woss opened this issue Aug 31, 2021 · 12 comments · Fixed by #117

Comments

@woss
Copy link

woss commented Aug 31, 2021

It's obvious that the T should not be there since it is unknown and generic. this should be typed differently. Anyone else knows how to make this library actually usable when used with Typescript?

Error:

[typescript] Using TypeScript version 4.2.4
[eslint] Using ESLint version 7.32.0
[typescript] Encountered 4 TypeScript issues:
[typescript] Error: ../../common/temp/node_modules/.pnpm/multiformats@9.4.6/node_modules/multiformats/types/codecs/json.d.ts:3:29 - (TS2304) Cannot find name 'T'.
[typescript] Error: ../../common/temp/node_modules/.pnpm/multiformats@9.4.6/node_modules/multiformats/types/codecs/json.d.ts:3:66 - (TS2304) Cannot find name 'T'.
[typescript] Error: ../../common/temp/node_modules/.pnpm/multiformats@9.4.6/node_modules/multiformats/types/codecs/json.d.ts:4:61 - (TS2304) Cannot find name 'T'.
[typescript] Error: ../../common/temp/node_modules/.pnpm/multiformats@9.4.6/node_modules/multiformats/types/codecs/json.d.ts:4:68 - (TS2304) Cannot find name 'T'.

Actual Code:

export const name: string;
export const code: 512;
export const encode: (data: T) => import("./interface").ByteView<T>;
export const decode: (bytes: import("./interface").ByteView<T>) => T;
export type BlockCodec<Code extends number, T_1> = import('./interface').BlockCodec<Code, T_1>;
//# sourceMappingURL=json.d.ts.map
@rvagg
Copy link
Member

rvagg commented Sep 2, 2021

The T is intended to carry over the original input type, e.g. PBNode for dag-pb.

The export here is roughly the same as for our other codecs that build on the same interfaces, including dag-json: https://unpkg.com/browse/@ipld/dag-json@8.0.0/types/index.d.ts, which is actively tested as being consumable as a TypeScript import: https://github.com/ipld/js-dag-json/blob/master/test/ts-use/src/main.ts

Can you just declare the codec like so: codec: BlockCodec<512, any>?

You can see where we get more specific than any in dag-pb: https://unpkg.com/browse/@ipld/dag-pb@2.1.9/types/src/index.d.ts

dag-jose is another codec that gets a bit specific, but there are many others that don't just accept any object that conforms to the IPLD data model. Hence the T.

@woss
Copy link
Author

woss commented Sep 2, 2021

wouldn't it be better to actually write this in pure TS and then compile it properly? JSDoc seems to do a bad job with generics. Providing the .d.ts files manually is not going to be easy to maintain and expectations of this project are huge and a lot of people will be switching from deprecated ones.

The reason why the typescript is complaining is that that .d.ts is not valid, since T is not considered a generic rather than a type that doesn't exist.

Here is the sample code with commonjs generic

export function withGeneric<T>(param: T): void {
  console.log(param)
}

and compiled commonjs:

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.withGeneric = void 0;
function withGeneric(param) {
    console.log(param);
}
exports.withGeneric = withGeneric;


// and gen.d.ts
export declare function withGeneric<T>(param: T): void;
//# sourceMappingURL=gen.d.ts.map

and esm

export function withGeneric(param) {
    console.log(param);
}

// gen.d.ts
export declare function withGeneric<T>(param: T): void;

I hope you see the difference.

@rvagg
Copy link
Member

rvagg commented Sep 2, 2021

the .d.ts files are generated by tsc --build, not manually

@Gozala can you weigh in on this one please?

@woss
Copy link
Author

woss commented Sep 2, 2021

your build uses ipjs?
why another builder? why not stick with standard builders and battle-tested approaches?

I understand the need for innovation and all but seriously, you are building the library that should be a defacto standard for any IPFS/multiformats operations, you should not use the builders for the sake of promoting group of people.

@woss
Copy link
Author

woss commented Sep 2, 2021

and on a really scary note, there is no lock file for the dependencies.

@woss
Copy link
Author

woss commented Sep 2, 2021

Plus the devDependencies have a deprecated package "cids": "^1.1.6", https://github.com/multiformats/js-cid#readme

@rvagg
Copy link
Member

rvagg commented Sep 2, 2021

@woss your critique is bordering on unhelpful and rude, please try and keep it constructive and try and stay focused on the issue at hand.

Regarding your comments unrelated to the original issue:

  • ipjs doesn't have anything to do with the typescript generation, it helps us bundle for maximum cjs and esm and it's developed by a number of us to get the right outcomes.
  • package-lock.json is up to you as a user, it's standard for library developers to not use them but where you are concerned about dependency creep as an application developer then that's where it's appropriate
  • cids is included to check compatibility with an older version

@woss
Copy link
Author

woss commented Sep 2, 2021

@rvagg it was aimed to be direct, not rude. You could also take it in a way that there are people out there who are very irritated by the libraries that are so essential and that they ( including myself ) need to do so much just to make our code work with yours.

I want to help, that's all.

@woss
Copy link
Author

woss commented Sep 2, 2021

If you guys think you don't need help, just say so, I'll fork this repo, rewrite it in TS as it should be and continue on my own.

@Gozala
Copy link
Contributor

Gozala commented Sep 2, 2021

The issue here is caused by a bug in typescript compiler microsoft/TypeScript#41258. As you can see from the source code type parameter is there:

/**
* @template T
* @type {BlockCodec<0x0200, T>}
*/
export const { name, code, encode, decode } = {
name: 'json',
code: 0x0200,
encode: json => new TextEncoder().encode(JSON.stringify(json)),
decode: bytes => JSON.parse(new TextDecoder().decode(bytes))
}

But in generated json.d.ts file T type parameter is no longer present.

Good news is TS bug appears fixed, if so resolving this issue would just require updating typescirpt version and regenerating .d.ts files.

@woss If you would like to help trying above suggestion would be my invite ?

Rewriting libraries in TS had been considered in the past and after evaluating pros and cons it was decided that doing TS via JSDOC syntax was the best compromise which is what this and other libs in IPFS ecosystem are doing today. While it may make sense to reevaluate pros and cons, not much has changed since last reevaluation and there are lot of more pressing issues to deal today. We still have have libraries that have not been entyped fully and bunch of @ts-ignores in the code base, I think help addressing those would be more productive use of everyones time who wishes for a better typescript support.

@rvagg
Copy link
Member

rvagg commented Sep 3, 2021

It took a bit more than updating TS unfortunately, it's this pattern of exporting the whole BlockCodec<T> (that we previously tried but reverted on js-dag-json and js-dag-cbor) that seems to be the problem, so in this we're just exporting the individual constituent pieces and it seems to work. But I'd appreciate a review: #117

@rvagg
Copy link
Member

rvagg commented Sep 3, 2021

Hopefully fixed in v9.4.7

https://unpkg.com/browse/multiformats@9.4.6/types/codecs/json.d.ts
vs
https://unpkg.com/browse/multiformats@9.4.7/types/codecs/json.d.ts

thanks for reporting the issue and bearing with us.

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

Successfully merging a pull request may close this issue.

3 participants