Skip to content

fix(natives): Vector3 will generate a tuple type instead of an array … #2439

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AleksanderEvensen
Copy link

@AleksanderEvensen AleksanderEvensen commented Mar 27, 2024

Goal of this PR

The vector3 type has until now been generated as an array type. In my opinion it should be generated as a tuple type instead.
The reason I think the tuple type is a better option is because it can be used properly with the spread operator in functions that takes in the x, y and z component (ref picture below)

image

How is this PR achieving the goal

Changing the codegen for dts files to parse the Vector3 type into the tuple type

This PR applies to the following area(s)

Natives

Successfully tested on

Has not been tested due to node-gyp not being able to compile the libclang bindings in native-doc-tooling

Game builds: N/A

Platforms: Windows

Checklist

  • Code compiles and has been tested successfully.
  • Code explains itself well and/or is documented.
  • My commit message explains what the changes do and what they are for.
  • No extra compilation warnings are added by these changes. (I won't expect it to generate any errors due to the small change, but I'll leave it uncheck since i haven't tested it)

Fixes issues

N/A

@github-actions github-actions bot added the triage Needs a preliminary assessment to determine the urgency and required action label Mar 27, 2024
@AvarianKnight
Copy link
Contributor

I think this would be better, though it would generate type errors where there were none before, but also not sure what the opinions on "compatibility breaks" are for types.

I think this change is better for the better as it should always return 3 values

@AleksanderEvensen
Copy link
Author

AleksanderEvensen commented Mar 27, 2024

I didn't even think about the compatibility breaking for the types 😅

@thorium-cfx thorium-cfx added the ScRT: JS Issues/PRs related to the JavaScript scripting runtime label Apr 18, 2024
@FabianTerhorst
Copy link
Contributor

I'm not sure if we can handle this change considering it causes to much compatibility issues.

@AleksanderEvensen
Copy link
Author

This could be done by a major version bump to the package (if that isn't off the table).

Using a tuple type in a function that takes an array as parameter doesn't yield an error in TypeScript.
The only instance where TypeScript yields an error is when an array is passed to a function where the parameter is a tuple type. I haven't seen any function in the type definitions for the natives where a function takes an array as input, so compatibility with other natives wouldn't break.

Error in user code wouldn't be a problem either, because if they're using a tuple type in their own functions they're also typecasting the array type anyway.

Would therefor argue that this wouldn't break so many things that I might have thought in the beginning.

Maybe some weird eslint rule would catch this case, but I didn't have the time to test this right now. If there is anything else that i might have missed, I would love to hear about it.

image

tsconfig used with example above:

{
    "compilerOptions": {
      "strict": true,
      "allowUnusedLabels": false,
      "allowUnreachableCode": false,
      "exactOptionalPropertyTypes": true,
      "noFallthroughCasesInSwitch": true,
      "noImplicitOverride": true,
      "noImplicitReturns": true,
      "noPropertyAccessFromIndexSignature": true,
      "noUncheckedIndexedAccess": true,
      "noUnusedLocals": true,
      "noUnusedParameters": true,
  
      "isolatedModules": true,
  
      "checkJs": true,
  
      "esModuleInterop": true,
      "skipLibCheck": true
    },
    "$schema": "https://json.schemastore.org/tsconfig",
    "_version": "2.0.0"
  }

@FabianTerhorst
Copy link
Contributor

Thanks for the explanation. When the array typed function parameter accepts the tuple as well its not really a compatibility issue.

@prikolium-cfx prikolium-cfx added the needs manual verification PRs that need manual verification by testing the change locally label Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs manual verification PRs that need manual verification by testing the change locally ScRT: JS Issues/PRs related to the JavaScript scripting runtime triage Needs a preliminary assessment to determine the urgency and required action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants