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

Generated types are not passing strict mode (I think) #187

Closed
HarelM opened this issue Feb 24, 2022 · 10 comments
Closed

Generated types are not passing strict mode (I think) #187

HarelM opened this issue Feb 24, 2022 · 10 comments

Comments

@HarelM
Copy link

HarelM commented Feb 24, 2022

Bug report

Seems like the generated code is not passing angular compilation for some reason.
Generated file can be found here:
https://unpkg.com/browse/maplibre-gl@2.1.6/dist/maplibre-gl.d.ts
The following issue seems to describe well the steps to reproduce:
maplibre/maplibre-gl-js#934
There are two unrelated issue which I'm not sure how to properly solve:

  1. The following at line 2260:
export declare type $ObjMap<T extends {}, F extends (v: any) => any> = {
--
[K in keyof T]: F extends (v: T[K]) => infer R ? R : never;
};
export declare type UniformValues<Us extends any> = $ObjMap<Us, <V>(u: Uniform<V>)

Produces the error:

error TS2344: Type 'Us' does not satisfy the constraint '{}'.
  Type 'unknown' is not assignable to type '{}'.

export declare type UniformValues<Us extends any> = $ObjMap<Us, <V>(u: Uniform<V>) => V>;

I'm not sure why.

  1. The following method which does not have a return value:
    https://github.com/maplibre/maplibre-gl-js/blob/51054671229c68d798fa06a64968aa35688f6a0f/src/ui/map.ts#L920
    Creates the following typings:

_createDelegatedListener(type: MapEvent, layerId: any, listener: any): {
    layer: any;
    listener: any;
    delegates: {
        mousemove: (e: any) => void;
        mouseout: (e: any) => void;
    };
    } | {
    layer: any;
    listener: any;
    delegates: {
        [x: string]: (e: any) => void;
        mousemove?: undefined;
        mouseout?: undefined;
    };
};

Which causes the following errors:

Error: node_modules/maplibre-gl/dist/maplibre-gl.d.ts:8635:4 - error TS2411: Property 'mousemove' of type 'undefined' is not assignable to string index type '(e: any) => void'.

        mousemove?: undefined;
        ~~~~~~~~~


Error: node_modules/maplibre-gl/dist/maplibre-gl.d.ts:8636:4 - error TS2411: Property 'mouseout' of type 'undefined' is not assignable to string index type '(e: any) => void'.

        mouseout?: undefined;
        ~~~~~~~~

Which I think make sense as the [x: string]: (e: any) => void; is incompatible with the other two mouse events.
Is this something that can/should be fixed in this repo or there's a need to change the original code some how?

Thanks again for this great project!!

@timocov
Copy link
Owner

timocov commented Apr 2, 2022

Hi @HarelM, I'm sorry for the late reply. Is it still the issue? I just tried to reproduce it and it seems to work now, most likely because of the fixes from your side. If so, is it possible to share what caused the issue? Thanks!

@HarelM
Copy link
Author

HarelM commented Apr 2, 2022

I've changed the code to workaround this issue but I think this is still an issue...

@timocov
Copy link
Owner

timocov commented Apr 2, 2022

Can you provide steps to reproduce the issue then please?

@HarelM
Copy link
Author

HarelM commented Apr 2, 2022

Take maplibre-gl code from before feb 24 (when the fixed was merge https://github.com/maplibre/maplibre-gl-js/pull/1023d).
And generate typescript definitions files:
npm run generate-typings
Take the d.ts file that was generated and try use it in an angular 13 project that uses strict mode (i.e. ng new my-project, npm i maplibre-gl, copy the d.ts file and try to build with ng build.)
I think this is was I did to reproduce the issue, not sure, it was more than a month ago... :-/

@timocov
Copy link
Owner

timocov commented Apr 6, 2022

@HarelM

The following at line 2260:
Produces the error:
I'm not sure why.

Well, looks like the types you were using here couldn't be compiled so that's why you had this error. You can paste that code to typescript playground and see that it will report an error:

image

The following method which does not have a return value:
maplibre/maplibre-gl-js@5105467/src/ui/map.ts#L920
Creates the following typings:

These types are created by the compiler itself and the tool isn't part of this process and cannot control them. If you have an error here, you need to provide proper types for this function manually.

I'm not sure why you don't see such errors (probably you don't run the compiler to type check? or maybe because of your compiler options ¯_(ツ)_/¯), but they were in your project before and should be fixed in the code.

@timocov timocov closed this as completed Apr 6, 2022
@HarelM
Copy link
Author

HarelM commented Jun 2, 2023

Hi, I know this is a bit late to the party, here's my scenario, I'm not sure what should be the solution here, but it would be great if this tool had a flag that can check if the output is passing strict typescript transpilation.
My scenario is as follows:

  1. I have a project that in order to introduce a strict: true flag to my tsconfig it will take a large effort, which, when I looked at what I get back, it doesn't seem like it improved the code much, in some places it will improve, but in most cases it's mainly noise, so setting strict: true doesn't seem like a return on investment.
  2. However, I do want the generated .d.ts file to pass a strict check so that the public API of my lib will not cause other libraries that use it to add skipLibCheck or set strictNullCheck: false, generally speaking I would like the generated code to pass strict scan.

In order to "fix" the above I did the following "hack": maplibre/maplibre-gl-js#2623 (added another tsconfig.json file and added tsc run to my CI).

It would be great if I could pass a commandline angument flag like --strict to make sure that the generated file is following the strict mode instead of doing it in my project, as I'm think this might be a common requirement to other projects as well.

Let me know if you would like me to open a new issue on this.
Thanks for all the hard work around this lib!

@timocov
Copy link
Owner

timocov commented Jun 3, 2023

Hey @HarelM,

The tool compiles (checks) generated dts file with compiler options defined in your provided tsconfig. Don't you have strict: true there?

Another little trick - if you don't provide preferredConfigPath option the tool will try to find closest to a file tsconfig.json file to load options from (the same way as the compiler does). So you can put tsconfig.json file in dist folder (for example) that would include desired options. I know it is not ideal, but at least something to start with without changing the tool.

I was thinking maybe to introduce a new flag, something like preferredConfigPath but for generated files, but not sure yet.

@HarelM
Copy link
Author

HarelM commented Jun 3, 2023

Naaa, adding a different config file isn't what I was thinking about, although this can be a valuable option for some projects, but for me, if I added another config file I can easily add a type check using this file, I was hoping for a more "built in" solution.
Something like running tsc and adding --strict to it somehow, configurably obviously.

@timocov
Copy link
Owner

timocov commented Jun 3, 2023

but for me, if I added another config file I can easily add a type check using this file

@HarelM What do you mean? Isn't it what you want to achieve?

I was hoping for a more "built in" solution

I don't think a built-in solution like "compile in strict mode" is valuable as it is too specific. Next day ppl will ask to add an option to compile with some other flags, and then another ones and we will come up with tens of different flags that would replicate tsconfig basically.

@HarelM
Copy link
Author

HarelM commented Jun 3, 2023

Yea, I understand your point, makes sense...

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

2 participants