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

Add TypeScript Definitions for JS API #55

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ByThePowerOfScience
Copy link

@ByThePowerOfScience ByThePowerOfScience commented Oct 31, 2024

This will allow some minor intellisense for coremod development.

@Jonathing
Copy link
Member

I'm also interested in adding type support for CoreMods, but I'm going to need a little more information on this since it's rather foreign to me.

  • This looks like an agnostic type support system to me. What IDEs does this support? Ideally I'd like it to be all of them.
  • Does this require CoreMods to apply any additional frameworks?

As of right now, CoreMods uses Nashorn 15.3, which does not include several newer JavaScript features that TypeScript usually relies on, so keep that in mind. I figure this might be better solved by writing JSDocs with type parameters rather than including a TypeScript library file.

@PaintNinja
Copy link
Contributor

PaintNinja commented Oct 31, 2024

I have some limited understanding of this, so answering some of your questions here. Proper clarification/documentation from @ByThePowerOfScience on how to use these type definitions when writing a JS coremod would be greatly appreciated though.

What IDEs does this support? Ideally I'd like it to be all of them.

JavaScript support in the Java IDE ecosystem isn't great. My guess is this'll work well in VS Code and NetBeans, maybe in IntelliJ Ultimate and as for Eclipse, I dunno. Personally I'd be happy even if this only helps Visual Studio Code - some support is better than none. Having a new VS Code window open for writing your JS CoreMods could still be a better experience than no type defs at all.

Does this require CoreMods to apply any additional frameworks?

No, it's a standalone TypeScript type definition file. You can attach it to your IDE to help it provide richer context of your CoreMods JS files. I haven't checked, but I think you could also do a JSDoc @use in your JS to tell it where the type definitions are to have it automatically picked up by your IDE.

As of right now, CoreMods uses Nashorn 15.3, which does not include several newer JavaScript features that TypeScript usually relies on, so keep that in mind.

TypeScript type definitions don't require TypeScript to be used - they can also be used for improved IDE support in plain JS. If using TypeScript, a compilation target set to ES5 would suffice for getting it working on Nashorn 15.3.

@Jonathing
Copy link
Member

In that case I'm ok with pulling this, although I do agree that additional clarification from our PR author here would be ideal.

@ByThePowerOfScience
Copy link
Author

ByThePowerOfScience commented Oct 31, 2024

In that case I'm ok with pulling this, although I do agree that additional clarification from our PR author here would be ideal.

It's as @PaintNinja said. Modern JS type checkers - like the native VSCode and IDEA linters - can use these type defintions even for raw JS code of any version.

For example, when validating these definitions, my coremod file looked like this:

/** @type CoreModInitializer */ // JSDoc for the function's return type
function initalizeCoreMod() {
  // ...
}

and IDEA was able to give me proper schema validation for the returned object.

In IDEA Ultimate, my dependencies' coremod JS files were consumed by the typechecker automatically, so I don't doubt the lib.d.ts file would be too.

Further type definitions for the allowed Java classes (Java.type(...)) could be generated using a Gradle plugin like https://github.com/bsorrentino/java2typescript, which generates type stubs for any specified Java classes. This could be used to provide typechecking for the ASM Node classes and ASMAPI as well.

@LexManos
Copy link
Member

I have no strong opinion on this, if it makes people's lives easier with minimal maintenance on our end, then i'm fine with it.
Would like to see some docs somewhere saying that lib.d.ts is the standard for putting these in.
As google tells me that lib.d.ts is typescript's internal file and you an disable it with a flag.

How difficult would it be to integrate the java2typescript plugin in this PR? As might as well do it all in one pass.

@ByThePowerOfScience
Copy link
Author

ByThePowerOfScience commented Oct 31, 2024

As google tells me that lib.d.ts is typescript's internal file and you an disable it with a flag.

You right; looking at DefinitelyTyped (the gold standard for 3rd party type references), the standard is index.d.ts under a namespace. I'll edit it.

How difficult would it be to integrate the java2typescript plugin in this PR? As might as well do it all in one pass.

I mean it's a Gradle plugin........... 😬 Nevermind, I was thinking of https://github.com/vojtechhabarta/typescript-generator. I'll take a look.

EDIT: Nevermind, neither work for what we need. One only generates method stubs, the other only generates fields. I don't have the knowledge needed to combine the two.

@ByThePowerOfScience
Copy link
Author

ByThePowerOfScience commented Oct 31, 2024

Ok I got it, but it's janky, manual, and it doesn't have the fields. I wouldn't recommend taking that commit; it'll probably be more confusing than just referencing the classes manually.

Either way, that part is far less necessary than the basic coremod schema being available somewhere.

Copy link
Member

Choose a reason for hiding this comment

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

ASMAPI will continue to get changes made to it as I continue to work on CoreMods. Is there any way we could automate this process? You did mention in your comment that this was a manual process.

Copy link
Author

Choose a reason for hiding this comment

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

It can technically be automated with the java2ts AP I mentioned, but it would probably be better to rig up a Python script with some regex.

I also made .d.ts files for the most-used ASM classes cause I got tired of having to pull up the InsnList docs constantly. it is also 1am i should stop

Copy link
Member

Choose a reason for hiding this comment

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

Naw, we're a java project using gradle/java toolchians. Better to write it in that then to require yet another toolset to build things.

@Jonathing
Copy link
Member

Since the work needed to make this efficient is a bit much at the moment, I'm not considering it an urgent pull for CoreMods 5.2.0.

Don't get me wrong though, I absolutely do want better IDE support for CoreMods. But if I pull this as-is, it will make maintainability a nightmare. A group of ASMAPI-related PRs which were recently merged have already slightly outdated the content in this PR.

@ByThePowerOfScience
Copy link
Author

Don't get me wrong though, I absolutely do want better IDE support for CoreMods. But if I pull this as-is, it will make maintainability a nightmare. A group of ASMAPI-related PRs which were recently merged have already slightly outdated the content in this PR.

We can just remove the ASMAPI part then. The thing that's absolutely required is including a schema for the expected return value of initializeCoreMods.

@Jonathing
Copy link
Member

Ok, then let's just remove the ASMAPI part for now then. I don't expect the schema for initializeCoreMods to change for a while, if at all.

@Jonathing
Copy link
Member

Jonathing commented Nov 18, 2024

As mentioned in MinecraftForge#10177, we are planning on deprecating CoreMods in the (somewhat?) near future. I would still like to make one last minor patch, 5.3, that includes the changes you've made here, but know that there probably won't be any changes to the implementation aside from any last-minute additions to ASMAPI. If you have the time, please cleanup this PR (basically just remove the ASMAPI stuff if you have it) and let me know so I can review it and have it merged in soon.

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

Successfully merging this pull request may close these issues.

4 participants