discord / erlpack

High Performance Erlang Term Format Packer
MIT License
220 stars 65 forks source link

Update to be compatible with Node 16 #40

Closed ExperiBass closed 3 years ago

ExperiBass commented 3 years ago

Ended up being three lines to remove.

Done on Linux 5.13.7-arch1-1 #1 SMP PREEMPT Sat, 31 Jul 2021 13:18:52 +0000 x86_64 GNU/Linux

fredkilbourn commented 3 years ago

Looking forward to seeing this merged, nice work!

Milo123459 commented 3 years ago

Vital for DJS' V13! Great work

DraftProducts commented 3 years ago

Is there an approximative date for the merge?

ExperiBass commented 3 years ago

@DraftProducts Most likely not since discord hasn't looked at this repo in a year, I'm thinkin of just making my fork the "main" active repo

Milo123459 commented 3 years ago

@DraftProducts Most likely not since discord hasn't looked at this repo in a year, I'm thinkin of just making my fork the "main" active repo

Please do! Maybe publish it as an npm package rather than having to install via git!

DraftProducts commented 3 years ago

Maybe a ping to jasoncitron ? He is the biggest contributor🤔

nerim4n commented 3 years ago

You can install GingkathFox's repo with npm i GingkathFox/erlpack

ExperiBass commented 3 years ago

@DraftProducts Most likely not since discord hasn't looked at this repo in a year, I'm thinkin of just making my fork the "main" active repo

Please do! Maybe publish it as an npm package rather than having to install via git!

@Milo123459 you can npm i GingkathFox/erlpack

fredkilbourn commented 3 years ago

Maybe open an issue on discord repo and reference this?

JasonDevTech commented 3 years ago

Hi guys, I've also wanted to know the status of this but I got a bit bored of waiting and decided to explore it on my own. I rewrote it to Typescript with browser support. Here's the repo if you guys want to check it out.

DraftProducts commented 3 years ago

Hi guys, I've also wanted to know the status of this but I got a bit bored of waiting and decided to explore it on my own. I rewrote it to Typescript with browser support. Here's the repo if you guys want to check it out.

It's a nice project, but I don't think that your package will give the same perfs as the official package with bindings 🤔

Milo123459 commented 3 years ago

@jasoncitron could you take a look at this?

fredkilbourn commented 3 years ago

@jhgg @adill @zorkian @mezz @fozzle @samschlegel @night - tagging some more discord people that are contributors, this is blocking full optimization with the new discord.js v13 library, which requires node v16, which requires this pull (or similar) to work.

ExperiBass commented 3 years ago

Merged #41 with this one since @Hazmi35 also updated deps

DraftProducts commented 3 years ago

We are now waiting about 10 days for this merge, I would make a fork of this repo and merge this PR myself. Thanks @GingkathFox for the work 👍

ExperiBass commented 3 years ago

Oh wow it's been 10 days already

Ig move to my fork?

ExperiBass commented 3 years ago

I've put in a ticket to detatch my fork from the main repo

Milo123459 commented 3 years ago

Your fork / published package doesn't work with DJS as it simply won't pick up your erlpack package.

Milo123459 commented 3 years ago

Also I think it's a bit scummy to make a PR changing who maintains the repo. Also remove the pnpm lock

icdevin commented 3 years ago

This has no chance of being accepted as it exists now since it tells users to install his own repo and changes the name and author/maintainers...if you're going to spin off your own, do so and feel free tell people but it would've been wiser to leave this PR with just the changes needed for Node v16.

Milo123459 commented 3 years ago

At this rate, either fix the PR with only needed changes, or someone make a new PR. It just makes no sense, changing the author, name, readme, etc to the name of your fork then attempting to make that change in the main repo? Doesn't make sense to me. Please revert to just needed changes

fredkilbourn commented 3 years ago

To be honest the best solution would be discord.js forking and maintaining this and not leaving users to fight for a fix. I've added an issue over there making the suggestion: https://github.com/discordjs/discord.js/issues/6418 upvote over there if you agree.

jhgg commented 3 years ago

Well you took a mergeable PR and made it totally unmergable. Feel free to re open with the original change set.

Milo123459 commented 3 years ago

Oh for gods sake :(

ExperiBass commented 3 years ago

Oh whoops, I'll re-push with the proper fixes, didn't realize that would affect it

I like how this happens after everything lol

Milo123459 commented 3 years ago

Oh whoops, I'll re-push with the proper fixes, didn't realize that would affect it

I like how this happens after everything lol

Not trying to be rude but how could you not see how this wouldn't affect it?

ExperiBass commented 3 years ago

Oh whoops, I'll re-push with the proper fixes, didn't realize that would affect it

I like how this happens after everything lol

Not trying to be rude but how could you not see how this wouldn't affect it?

  • Change Authors

  • Renamed

  • Add a pnpm-lock (unneeded)

  • Then proceeded to make the main repo look like the fork?

I didn't realize my fork was edited on the master, I thought it had created a new branch with the fixes

Also the pnpm lock should've been ignored, I use pnpm instead of npm

Milo123459 commented 3 years ago

Pnpm lock should still be ignored