citizenfx / fivem

The source code for the Cfx.re modification frameworks, such as FiveM, RedM and LibertyM, as well as FXServer.
https://cfx.re/
3.53k stars 2.08k forks source link

v8 exports type definitions conflict with @types/node #1910

Open pedr0fontoura opened 1 year ago

pedr0fontoura commented 1 year ago

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.

blattersturm commented 1 year ago

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.

pedr0fontoura commented 1 year ago

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

joelwurtz commented 1 year ago

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: {
            ...
        },
        ...
    }
}
AvarianKnight commented 1 year ago

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?

pedr0fontoura commented 1 year ago

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.

thelindat commented 1 week ago

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?

pedr0fontoura commented 1 week ago

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;
  2. Add a new line with

    declare var FXExports: CitizenExports;
  3. 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.