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

Autogenerate TS typedefs for Core #5429

Merged
merged 22 commits into from
Dec 5, 2023
Merged

Autogenerate TS typedefs for Core #5429

merged 22 commits into from
Dec 5, 2023

Conversation

arthuro555
Copy link
Contributor

Adds a script that parses the Bindings.idl to generate a TypeScript DTS file, as a first step for a potential migration of the IDE from Flow to TypeScript.

@arthuro555 arthuro555 requested a review from 4ian as a code owner June 21, 2023 13:14
Copy link
Owner

@4ian 4ian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent stuff! Will be very helpful for the future or for any tooling using libGD.js directly.
The parser seems fairly concise which is helpful to avoid bringing big dependencies or complex stuff. We could probably adapt the flow generation code to be the same.

@D8H once merged, let's use this in other repos :)

GDevelop.js/scripts/generate-dts.mjs Outdated Show resolved Hide resolved
GDevelop.js/scripts/generate-dts.mjs Outdated Show resolved Hide resolved
GDevelop.js/scripts/generate-dts.mjs Outdated Show resolved Hide resolved
GDevelop.js/scripts/generate-dts.mjs Outdated Show resolved Hide resolved
GDevelop.js/scripts/generate-dts.mjs Outdated Show resolved Hide resolved
GDevelop.js/scripts/generate-dts.mjs Outdated Show resolved Hide resolved
GDevelop.js/scripts/generate-dts.mjs Outdated Show resolved Hide resolved
GDevelop.js/scripts/generate-dts.mjs Outdated Show resolved Hide resolved
Copy link
Owner

@4ian 4ian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arthuro555 if you can fix the requested changes, then we can merge this :)

@arthuro555
Copy link
Contributor Author

Sorry for the delay, I had left on vacation without my laptop ✌️
I've done the requested changes 👍

@arthuro555 arthuro555 requested a review from 4ian July 15, 2023 00:05
Copy link
Owner

@4ian 4ian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! The npm install on Travis is failing, any idea why?

@arthuro555
Copy link
Contributor Author

Looks good! The npm install on Travis is failing, any idea why?

Hmm git probably messed up package-lock.json when merging with upstream, I regenerated it, hopefully that fixes it.

@arthuro555
Copy link
Contributor Author

Looks like since the emsdk environement registers an old node/npm version, we needed a package-lock v1 for it to install properly

@4ian
Copy link
Owner

4ian commented Jul 21, 2023

Nice!
I did try locally but got:


Running "shell:generateTSTypes" (shell) task
internal/modules/cjs/loader.js:981
  throw new ERR_REQUIRE_ESM(filename);
  ^

Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /Users/florian/Projects/F/GD/GDevelop.js/scripts/generate-dts.mjs
    at Object.Module._extensions..mjs (internal/modules/cjs/loader.js:981:9)
    at Module.load (internal/modules/cjs/loader.js:790:32)
    at Function.Module._load (internal/modules/cjs/loader.js:703:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:999:10)
    at internal/main/run_main_module.js:17:11
Warning: Command failed: node scripts/generate-dts.mjs
internal/modules/cjs/loader.js:981
  throw new ERR_REQUIRE_ESM(filename);
  ^

Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /Users/florian/Projects/F/GD/GDevelop.js/scripts/generate-dts.mjs
    at Object.Module._extensions..mjs (internal/modules/cjs/loader.js:981:9)
    at Module.load (internal/modules/cjs/loader.js:790:32)
    at Function.Module._load (internal/modules/cjs/loader.js:703:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:999:10)
    at internal/main/run_main_module.js:17:11
 Use --force to continue.

node -v gives v12.9.1. Can you confirm what Node version you have locally when in the Emscripten environment?
Looks like the CI might be using a more recent version.

@4ian
Copy link
Owner

4ian commented Sep 6, 2023

I suggest I convert the script back to "good old" require, so that this is "safe" with older Node.js version - whatever the system/CI/emscripten version. So we can then merge this and ensure it always work.

@arthuro555
Copy link
Contributor Author

Node supports esm since v14. v14, v16 in 5 days from today, and all versions below, all have reached end of life. Instead of doing additional work to convert this script back to cjs "just in case", we should rather use the energy to update CIs & the like from very outdated node versions to an actually supported version of node.

@arthuro555
Copy link
Contributor Author

For emscripten specifically, I just tried and GDevelop.js builds perfectly fine (albeit with a few C++17 deprecation warnings for using binary_function) under the latest version of emscripten, where emsdk's environment uses Node v16.

@4ian
Copy link
Owner

4ian commented Sep 7, 2023

If you're fine for this PR to land later, we can then take the time to upgrade Emscripten.

@4ian 4ian changed the base branch from master to experimental-build/emscripten-upgrade November 21, 2023 18:39
@4ian
Copy link
Owner

4ian commented Dec 5, 2023

So Travis CI failing because lock file is probably too old.
And Semaphore failing because... it never manages to do the newIDE tests (??).

Let's regenerate, again, the package-lock.json with at least Node.js 16 bundled with Emscripten?
EDIT: done

@4ian 4ian changed the base branch from experimental-build/emscripten-upgrade to master December 5, 2023 16:11
@4ian 4ian merged commit 4f04190 into 4ian:master Dec 5, 2023
8 checks passed
@4ian
Copy link
Owner

4ian commented Dec 5, 2023

Finally merged! I almost think we could generate the Flow types like you did with a simple parser, rather than relying on the complicated webidl-tools-flow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants