Fix typing error when consuming maplibre-gl with TypeScript compiler …

[neodescis]

Simple typing change that closes #2588. Note that the removal of | null makes the class consistent with all others in the file. With this change, I'm able to consume the library from a project without strict: false set in its tsconfig.

Launch Checklist

• Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
• Briefly describe the changes in this PR.
• Link to related issues.
• Include before/after visuals or gifs if this PR includes visual changes.
• Write tests for all new functionality.
• Document any changes to public APIs.
• Post benchmark scores.
• Add an entry to CHANGELOG.md under the ## main section.

[neodescis]

I think if we want to guard against a problem like this in the future, maybe it would be good to add a dummy project that references the built dist folder and is configured strict: true, and then add a check that verifies it builds? I'm not really sure how to go about that though.

[HarelM]

Can we set yhe tsconfig somehow to guard for this?

[neodescis]

Can we set yhe tsconfig somehow to guard for this?

I think having strict: false in the tsconfig means it specifically won't complain about this? Setting it to true is going to open a big can of worms though. That's why I mentioned adding a separate dummy project just to ensure something else with that set can consume the typings.

[HarelM]

Can't we just add strictNullCheck? Would that solve this specific case and guard for that only as a first step?

[neodescis]

Happy to try it and see what happens. I'll report back.

[HarelM]

THANKS!!

[neodescis]

Well, adding strictNullChecks: true does indeed catch the problem, but unfortunately it also catches 1900 lines worth of other problems too. I was hopeful that strictFunctionTypes might also catch the problem, but it does not.

[HarelM]

Interesting, So how come the line you fixed here is the only one needed in order to solve this issue?

[neodescis]

Adding | null here directly conflicts with the return type defined in the parent class BaseValue<T>, which does not specify that. I'm kind of surprised that this builds at all in MapLibre, even with strict: false. I tried removing that entirely from the tsconfig, but it still builds. What's more, the parameter of the set() function in that same class also doesn't match the base class, but I can build my own project against that without issue.

So, all I can figure out is that TypeScript must be performing different (less strict) checks on dependencies (.d.ts files?), but when building a project against these typings, this return type mismatch is still something it cares about. I'm not sure why though.

[HarelM]

I don't understand :-( there are other places in the same file with the same problem, how come only this affects dependent libraries...
While I understand that this fixes the issue it seems it's only a patch.
Not sure what to do with it...

[neodescis]

I agree wholeheartedly. This is the only occurrence of getDefault() not matching the base class's typings, but the parameters to set() not matching and still working is strange. This issue is blocking me from using 3.x, but I do want to see a long-term solution. What do you think of getting the typings to work, and then we can work on adding a dummy project with strict: true to the list of merge checks? I would be happy to work on it, though I may need a little guidance as I've never made a github merge check before.

As an aside, I cannot simply turn off strict: true as I too am maintaining a library, and that would break a bunch of projects.

[HarelM]

Let me think about it, maybe I can do an effort to clean this project and not introduce a "weird test".

[neodescis]

I'd also be happy to work toward strict: true if that's the direction we want to take! It'd probably be better, but it will take some doing. Maybe we can introduce one check at a time or something.

[neodescis]

I'm wondering if this exists because of a combination of this open TypeScript issue, having strict null checking turned off, and explicitly declaring the return type of the subclass's function to include null.

Also, when I have the maplibre-gl-js project open in vscode and then open the generated .d.ts file, it underlines the line in red and shows the exact same error!

[neodescis]

I found something even more strange. If I change the getDefault() function to this:

getDefault(): WebGLVertexArrayObject | string {
    return '';
}

Everything works. The typings are generated "correctly" (getDefault(): WebGLVertexArrayObject | string;), and my project that is built with strict: true also builds. So, it definitely seems to be something with strictNullChecks. To confirm this, I tried all the other strict checks but didn't see any errors for that function when generating typings.

[HarelM]

I've added strictNullCheck and got tons of errors.
I can start a branch to coordinate work on that if you'd like.
It's a lot of work, but it will make the code a lot better at the end...

[HarelM]

I started a branch and opened a PR.
You are welcome to join the fun :-)

[HarelM]

I think i know why this is the only issue the we see.
This library is exposed using dts builder, which only exposes a declaration file and this file only exposes the public API, without any implementation, so the only part that is problematic and public and part of the declaration is this part, apparently.
So it makes sense, to me at least.
We can do a strict typecheck only on the generated d.ts file to avoid this kind of issues in the future, without fixing all the 1900 errors, but I think fixing those will make the code better, so I tend to say it's worth the effort.

[neodescis]

I started a branch and opened a PR. You are welcome to join the fun :-)

Thanks very much! I'll jump in as soon as I can.

I think i know why this is the only issue the we see. This library is exposed using dts builder, which only exposes a declaration file and this file only exposes the public API, without any implementation, so the only part that is problematic and public and part of the declaration is this part, apparently. So it makes sense, to me at least. We can do a strict typecheck only on the generated d.ts file to avoid this kind of issues in the future, without fixing all the 1900 errors, but I think fixing those will make the code better, so I tend to say it's worth the effort.

That makes sense to me too. Have you verified if that's the only public strict failure? Just curious.

[HarelM]

Can you check if adding the following files help future proofing this situations:

1. add a file called tsconfig.dist.json in the root of this repo with the following content:

{
  "extends": "./tsconfig.json",
  "compilerOptions": {
    "strictNullChecks": true,
  },
  "include": [
    "dist/maplibre-gl.d.ts"
  ],
  "exclude": []
}

2. in package.json replace the typecheck command with: "typecheck": "tsc --noEmit && tsc --project tsconfig.dist.json"

From a small test I made this shows the error before fixing it, and the error is gone after the fix.
Please let me know if this solves this case.
If so, I think it's a better solution for this issue at this point in time, you can see my comments in the other PR as to way I'm not sure fixing all the null checks is the way to go...

[neodescis]

This works for me! I also agree with your assessment. However, I did notice a problem with typecheck that had me confused for a minute. I'll comment on your PR though.