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.

Sign up for GitHub

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

Jump to bottom

CoreMods 5.2: ES6 language support, additional ASMAPI methods, and all-around cleanup #64

Closed
wants to merge 1 commit into from
Closed

CoreMods 5.2: ES6 language support, additional ASMAPI methods, and all-around cleanup #64

wants to merge 1 commit into from

Conversation

Jonathing
Copy link
Member

@Jonathing Jonathing commented Nov 2, 2024 •
edited
Loading

This PR is the culmination. The quintessential climax. The king. All of the above. I humbly (and proudly) request that this PR marks CoreMods 5.2.0. Once this PR is merged, I will resume work on bumping CoreMods on all applicable versions of Minecraft Forge.

Merging Process

For this PR, I would like approvals from both @LexManos and @PaintNinja in order to make sure I am not doing anything majorly stupid. However, I've been testing all of these changes so far in my own side projects and can assure you that they work well. Over the next 1-2 days, I'll be doing additional testing and making sure all the code is sane and nothing is broken.

Features

  • New README document that better describes the purpose of the JS CoreMods framework.
  • Enabled ES6 language support.
  • Add helper methods for creating and finding method and field nodes #63 is included.
  • New module-info.java that is backwards compatible with existing versions of Forge (tested on 1.19.2).
  • Bumped Nashorn to 15.4.
  • Complete code reformatting, assisted by IntelliJ IDEA Community.
  • A complete re-arrangement of the methods and enums in ASMAPI to make maintainability less of a pain in the ass.
  • Deprecated the old version of buildMethodCall() to be removed in CoreMods 6.0.0.
  • Cleaned up and polished existing documentation in ASMAPI.
  • Added Javadocs to all classes in the project, along with comments for future reference.
  • All-around code cleanup, especially of ASMAPI.
  • The tests now compile with the help of Unsafe. Fixes Tests are broken for CoreMods #57.

Future Plans

Although not part of this PR for CoreMods 5.2.0, there are several points of interest that I want to touch on in the next version of CoreMods.

  • Including better IDE linting of the JavaScript framework (see Add TypeScript Definitions for JS API #55).
  • Reviving the tests. Even though they compile now with the help of Unsafe, I do not have them running yet, and they are older tests that merely exist inside of isolated methods with the @Test annotation.
  • For 6.0.0: properly handling CoreMod errors by crashing the game with as much information as possible, rather than leaving it to rot with an untransformed node while the game continues. This could be an opt-out system like how Mixin has require = 0.
  • For SecureModules/FML/ModLauncher: full class dumping on after transformation.

@Jonathing

This comment was marked as outdated.

Sign in to view
@Jonathing Jonathing requested a review from a team November 2, 2024 14:58
@Jonathing Jonathing marked this pull request as ready for review November 3, 2024 18:41
@Jonathing
Copy link
Member Author

This PR has been completed, tested (Forge 43.4.4 with some changes that I will include in a PR), and polished to the best of my ability. It is now ready for review. I have updated the PR description to include as much detail about the included changes as possible.

@Jonathing Jonathing changed the title Final (for real this time) CoreMods cleanup, 5.2.0 CoreMods 5.2: Code and Documentation cleanup, additional ASMAPI methods, and polish Nov 3, 2024
@Jonathing Jonathing changed the title CoreMods 5.2: Code and Documentation cleanup, additional ASMAPI methods, and polish CoreMods 5.2: Code and Documentation cleanup, additional ASMAPI methods, and extra polish Nov 3, 2024
@Jonathing Jonathing changed the title CoreMods 5.2: Code and Documentation cleanup, additional ASMAPI methods, and extra polish CoreMods 5.2: ES6 language support, additional ASMAPI methods, and all-around cleanup Nov 3, 2024
@Jonathing Jonathing mentioned this pull request Nov 4, 2024
Merged
PaintNinja
PaintNinja requested changes Nov 4, 2024
View reviewed changes
Copy link
Contributor

@PaintNinja PaintNinja 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, just a few small things.

Please also make the static ThreadLocal field final in CoreModTracker.

README.md Outdated Show resolved Hide resolved
coremods-test/src/test/javascript/testcore2mod.js
Comment on lines +1 to +4
/*
* Copyright (c) Forge Development LLC
* SPDX-License-Identifier: LGPL-2.1-only
*/
Copy link
Contributor

@PaintNinja PaintNinja Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would personally prefer if these licence headers were skipped for JS files, but leaving Lex to decide on that. Note that Nashorn has a fast-path for files under 512 bytes, so adding licence headers may slow down load times.

Copy link
Member

Choose a reason for hiding this comment

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

Honestly I have no opinion on this, I don't think it matters tho especially for test scripts as they should be small and are only used for tests not production.

Copy link
Member Author

Choose a reason for hiding this comment

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

What about the CoreMods in Forge itself?

Copy link
Member

Choose a reason for hiding this comment

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

All of them are already over 512B so its a non-issue having the headers in there.

src/main/java/net/minecraftforge/coremod/transformer/CoreModBaseTransformer.java Show resolved Hide resolved
LexManos
LexManos reviewed Nov 4, 2024
View reviewed changes
coremods-test/build.gradle Outdated Show resolved Hide resolved
@Jonathing
CoreMods 5.2.0
347bbde 
Full Changelog:
https://gist.github.com/Jonathing/c3ad28b2a048ac839a7baba5417ee870

The key features are:
- ES6 language support
- Thoroughly updated ASMAPI, with full documentation
- Bug fixes (some optional for backwards-compatibility)
- Partial internal code cleanup
@Jonathing Jonathing force-pushed the feat/jonathing/master/final-cleanup-for-real branch from 94bd6c7 to 347bbde Compare November 5, 2024 01:40
@LexManos
Copy link
Member

LexManos commented Nov 5, 2024

Merged in e7503dd manually so I could tag.

@LexManos LexManos closed this Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LexManos LexManos LexManos left review comments

@PaintNinja PaintNinja PaintNinja approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tests are broken for CoreMods
3 participants
@Jonathing @LexManos @PaintNinja