Fixed the enchantment table "ready" event, which has been broken for years.

[zisis912]

What used to happen was that the "ready" event would get emitted when all the level values were above 0, however 0 means that a packet hasnt been sent yet

[pokahs]

very cool yeah getting enchantment name by string is just way more convenient also nice fix :p

[u9g]

Please seperate this pr into two seperate prs, since it has two distinctly different changes.

[zisis912]

One thing I am worried about is that the packets for level could be sent before the packets for expected.enchant, resulting in something like

[
  { level: 5, expected: { enchant: 'unbreaking', level: -1 } },
  { level: 11, expected: { enchant: null, level: -1 } },
  { level: 30, expected: { enchant: null, level: -1 } }
]

Does anyone know the order with which the packets get sent? Maybe it would be smarter to make the "ready" event reliant on expected.level, depending on the order of the packets

[zisis912]

Please seperate this pr into two seperate prs, since it has two distinctly different changes.

i dont know how to do that 😭 I made a commit to my fork and it got auto-added to the pull request

I think it's okay if we keep this as a 2 in 1, just a general enchanting library fix and overhaul to modernise it.

[zisis912]

After some stress testing I saw that its possible for the level to update without the expected.level updating, so I made sure to change the condition to check for the latter.

[u9g]

Please seperate this pr into two seperate prs, since it has two distinctly different changes.

i dont know how to do that 😭 I made a commit to my fork and it got auto-added to the pull request

I think it's okay if we keep this as a 2 in 1, just a general enchanting library fix and overhaul to modernise it.

That's exactly the problem, the "overhaul" is a breaking change to anyone who uses this now.

[zisis912]

Please seperate this pr into two seperate prs, since it has two distinctly different changes.

i dont know how to do that 😭 I made a commit to my fork and it got auto-added to the pull request

I think it's okay if we keep this as a 2 in 1, just a general enchanting library fix and overhaul to modernise it.

That's exactly the problem, the "overhaul" is a breaking change to anyone who uses this now.

JavaScript is a language that requires one to actively update their code to the newest documentation, atleast as far as web development goes. I don't think this would go against that principle.

Also I would like to mention that there is not a single person that used the enchanting library, since it was broken this whole time. Think of this as a new feature getting added, instead of an old one being modified. Enchanting did not really exist in a usable form up to this point.

And the few people that did use it are certainly smart and involved enough to know where the issue lies

[rom1504]

1.8.8 enchanting test is failing
can you please fix it ?

[zisis912]

can you please merge this and ignore 1.8.8, the enchanting module does not work on ANY major version right now. While we are worrying that 1.8.8 might be broken, every major release in the last few years is broken, and this is a fix for them.

I cant manually do this tweak in each of my projects, pls just merge it

[rom1504]

We can't merge PR which break the CI or it breaks merging any further PR

If you care about this change then take a few minutes to finish it