fivem icon indicating copy to clipboard operation
fivem copied to clipboard

v8 exports type definitions conflict with @types/node

Open pedr0fontoura opened this issue 2 years ago • 7 comments

#1895 Revealed a conflict between the project type definitions and @types/node.

It causes the same error the PR aims to prevent: TS2403: Subsequent variable declarations must have the same type.

It's probably the reason why exports was previously typed as any.

Using "skipLibCheck": true in the tsconfig.json fixes the error and seems to be a somewhat adequate solution, but it reduces type accuracy. More information about the skipLibCheck flag can be found at https://www.typescriptlang.org/tsconfig#skipLibCheck

I'm not sure what would be the best approach here. Reverting the PR would be the easiest fix but considering that FiveM uses a modified version of Node, the type definitions from @citizenfx/* should be the preferred ones in my opinion.

pedr0fontoura avatar Mar 18 '23 06:03 pedr0fontoura

Given that the naming for exports also tends to conflict with bundlers (requiring an explicit globalThis.exports), it could make sense to have the main runtime set both exports and a new name fxExports = exports or so, keeping the type definition as any for the original one, but having the new type definition for the non-conflicting name.

I'd like some feedback about that before actually going ahead with such, though.

blattersturm avatar Mar 18 '23 09:03 blattersturm

Sounds like a good solution to me. I'll reach out to some js users and ask for their input.

pedr0fontoura avatar Mar 18 '23 10:03 pedr0fontoura

seems like a good solution, also you could add a specific type for client / server like this (still on any) :

type fxClientExports = any;
type fxServerExports = any;

declare var fxExports: fxClientExports;
declare var fxExports: fxServerExports;

This would allow to override this type by project by doing the following override (not sure about the syntax, but it's definitly possible) :

declare module "@citizenfx/server" {
    type fxServerExports = {
        oxmysql: {
            ...
        },
        ...
    }
}

joelwurtz avatar Mar 18 '23 12:03 joelwurtz

Given that the naming for exports also tends to conflict with bundlers (requiring an explicit globalThis.exports), it could make sense to have the main runtime set both exports and a new name fxExports = exports or so, keeping the type definition as any for the original one, but having the new type definition for the non-conflicting name.

I'd like some feedback about that before actually going ahead with such, though.

This seems like a valid solution, should the documentation be updated to favor fxExports over exports if this change gets added?

AvarianKnight avatar Mar 18 '23 15:03 AvarianKnight

seems like a good solution, also you could add a specific type for client / server like this (still on any) :

type fxClientExports = any;
type fxServerExports = any;

declare var fxExports: fxClientExports;
declare var fxExports: fxServerExports;

If the types are set up correctly, you shouldn't need to have different types for client/server. You're supposed to reference only one of the @citizenfx/ packages in the tsconfig.json

This would allow to override this type by project by doing the following override (not sure about the syntax, but it's definitly possible) :

declare module "@citizenfx/server" {
    type fxServerExports = {
        oxmysql: {
            ...
        },
        ...
    }
}

https://github.com/citizenfx/fivem/pull/1895 has an example of extending the exports interface. In this case, extending the typings seems to be better than overriding them.

pedr0fontoura avatar Mar 19 '23 20:03 pedr0fontoura

I'm going to blow my brains out if I have to continue using skipLibCheck or some several-years-out-of-date package version without this type conflict. Any update?

thelindat avatar Oct 05 '24 04:10 thelindat

The solution proposed here is a good one.

It would require only a small set of changes:

  1. Revert the exports type to any here
declare var exports: any;
  1. Add a new line with
declare var FXExports: CitizenExports;
  1. Then, expose the new typed exports variable on main.js.
global.FXExports = global.exports;

I can't PR it because I no longer have the repo set up. I haven't tested the code above, but it shouldn't be too different from this.

pedr0fontoura avatar Oct 05 '24 20:10 pedr0fontoura