Skip to content

Update to be compatible with Node 16#40

Closed
0xf0xx0 wants to merge 9 commits intodiscord:masterfrom
0xf0xx0:master
Closed

Update to be compatible with Node 16#40
0xf0xx0 wants to merge 9 commits intodiscord:masterfrom
0xf0xx0:master

Conversation

@0xf0xx0
Copy link
Copy Markdown

@0xf0xx0 0xf0xx0 commented Aug 3, 2021

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

@ghost ghost mentioned this pull request Aug 6, 2021
@fredkilbourn
Copy link
Copy Markdown

Looking forward to seeing this merged, nice work!

@Milo123459
Copy link
Copy Markdown

Vital for DJS' V13! Great work

@DraftProducts
Copy link
Copy Markdown

Is there an approximative date for the merge?

@0xf0xx0
Copy link
Copy Markdown
Author

0xf0xx0 commented Aug 8, 2021

@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
Copy link
Copy Markdown

@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
Copy link
Copy Markdown

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

@nemoralis
Copy link
Copy Markdown

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

@0xf0xx0
Copy link
Copy Markdown
Author

0xf0xx0 commented Aug 9, 2021

@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
Copy link
Copy Markdown

Maybe open an issue on discord repo and reference this?

@JasonDevTech
Copy link
Copy Markdown

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
Copy link
Copy Markdown

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
Copy link
Copy Markdown

@jasoncitron could you take a look at this?

@fredkilbourn
Copy link
Copy Markdown

@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.

@0xf0xx0
Copy link
Copy Markdown
Author

0xf0xx0 commented Aug 12, 2021

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

@DraftProducts
Copy link
Copy Markdown

DraftProducts commented Aug 13, 2021

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 👍

@0xf0xx0
Copy link
Copy Markdown
Author

0xf0xx0 commented Aug 13, 2021

Oh wow it's been 10 days already

Ig move to my fork?

@0xf0xx0
Copy link
Copy Markdown
Author

0xf0xx0 commented Aug 13, 2021

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

@Milo123459
Copy link
Copy Markdown

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

@Milo123459
Copy link
Copy Markdown

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

@icdevin
Copy link
Copy Markdown

icdevin commented Aug 13, 2021

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
Copy link
Copy Markdown

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
Copy link
Copy Markdown

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: discordjs/discord.js#6418 upvote over there if you agree.

@jhgg
Copy link
Copy Markdown
Contributor

jhgg commented Aug 13, 2021

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

@jhgg jhgg closed this Aug 13, 2021
@Milo123459
Copy link
Copy Markdown

Oh for gods sake :(

@0xf0xx0
Copy link
Copy Markdown
Author

0xf0xx0 commented Aug 14, 2021

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
Copy link
Copy Markdown

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?

@0xf0xx0
Copy link
Copy Markdown
Author

0xf0xx0 commented Aug 14, 2021

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

@0xf0xx0 0xf0xx0 mentioned this pull request Aug 15, 2021
@Milo123459
Copy link
Copy Markdown

Pnpm lock should still be ignored

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.

9 participants