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

Error at build #2588

Closed
javissimo opened this issue May 25, 2023 · 13 comments · Fixed by #2623
Closed

Error at build #2588

javissimo opened this issue May 25, 2023 · 13 comments · Fixed by #2623
Labels
PR is more than welcomed Extra attention is needed

Comments

@javissimo
Copy link

maplibre-gl-js version: 3.0.0

Steps to Trigger Behavior

  1. Add maplibre-gl as a dependency
  2. pnpm i
  3. pnpm run build

Expected behavior: application compiles without error

Actual behavior:

Error: node_modules/.pnpm/maplibre-gl@3.0.0/node_modules/maplibre-gl/dist/maplibre-gl.d.ts:1145:2 - error TS2416: Property 'getDefault' in type 'BindVertexArray' is not assignable to the same property in base type 'BaseValue<WebGLVertexArrayObject>'.
  Type '() => WebGLVertexArrayObject | null' is not assignable to type '() => WebGLVertexArrayObject'.
    Type 'WebGLVertexArrayObject | null' is not assignable to type 'WebGLVertexArrayObject'.
      Type 'null' is not assignable to type 'WebGLVertexArrayObject'.

1145  getDefault(): WebGLVertexArrayObject | null;

I've been using version 3.0.0-pre.5 so far without encountering this error.

@HarelM
Copy link
Collaborator

HarelM commented May 25, 2023

As a workaround you can set strictNullCheck to false in your tsconfig file.

@javissimo
Copy link
Author

It's a big project and I like having strict rules, I'm not in favor of being less restrictive due to a dependency. For now I'll stick with the beta!

Maybe allowing the type to be BaseValue<WebGLVertexArrayObject | null> would suffice.

@HarelM
Copy link
Collaborator

HarelM commented May 25, 2023

Feel free to submit a PR.
I personally don't see a benefit in littering the code with | null, but that maybe only my preferences...

@HarelM HarelM added the PR is more than welcomed Extra attention is needed label May 25, 2023
@birkskyum
Copy link
Member

birkskyum commented May 25, 2023

It's probably a good idea to work towards enabling strict: true in maplibre project - it appear to catch quite a lot of inconsistencies

@neodescis
Copy link
Collaborator

neodescis commented May 25, 2023

In addition to the above error, I'm also seeing the following:

node_modules/maplibre-gl/dist/maplibre-gl.d.ts:11075:1 - error TS1383: Only named exports may use 'export type'.

11075 export type * from "@maplibre/maplibre-gl-style-spec";

It seems that "export type *" is only supported in TypeScript 5.0 and up. If we want to require that, we may want to document it somewhere. FWIW, TypeScript 5.0 is only supported in the latest version of Angular (released a couple weeks ago). Not sure about other UI frameworks off hand.

@neodescis
Copy link
Collaborator

PR fixes the first posted problem. I think the second problem I mentioned probably warrants its own issue for discussion? Let me know and I'll create one.

@HarelM
Copy link
Collaborator

HarelM commented May 26, 2023

You can overcome typescript 5 issue by adding skipLibCheck flag.
I've documented this in ngx-maplibre-gl that I also maintain.
Since this is using latest language features I don't see this as a problem, documenting it won't hurt though.

@neodescis
Copy link
Collaborator

It should probably be noted that the original issue above can also be overcome by adding skipLibCheck: true

@HarelM
Copy link
Collaborator

HarelM commented May 31, 2023

I've opened a PR that will solve more than just this error.
I've highlighted the path forward.
See #2620
Feel free to submit PRs against that branch.

@anneb
Copy link

anneb commented Jun 14, 2023

You can overcome typescript 5 issue by adding skipLibCheck flag. I've documented this in ngx-maplibre-gl that I also maintain. Since this is using latest language features I don't see this as a problem, documenting it won't hurt though.

As one might guess, Maplibre is also being used outside of the latest version of Angular. I do not see why you would use language features that are only available in the latest and greatest for a library that is for general use? Maybe rename to mapblibre-gl-angular or something?

Does the requirement to use 'skipLibCheck: true' also skip checks for all other libraries in use, just because maplibre is using the latest and greatest but not widely available future language features?

Wouldn't it be nicer and more compatible if the library supports a compiler version that is in general use?

As I am not a contributor, I should not complain about the hard work, but I hope someone sees the point. Or am I missing something?

@birkskyum
Copy link
Member

@anneb , the changes of #2623 are released as part of v3.1.0 - do you experience this issue with that version still?

@anneb
Copy link

anneb commented Jun 14, 2023

While preparing a setup to demo the problem, I think my 'error TS1383: Only named exports may use 'export type'' was caused by the fix I used to resolve the problem in the previous (< v3.1.0) version. Thanks for asking me to double-check.

@HarelM
Copy link
Collaborator

HarelM commented Jun 15, 2023

If you have a different solution to exporting the style-spec types that doesn't require typescript 5 you are more than welcome to contribute your time and effort to resolve this.
As you said, complaining is not helpful to anyone.
You are also welcome to use an older version until this is resolved, no one is forcing you to upgrade...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment