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

fix: switch to @node-ipc/compat #7063

Closed
wants to merge 3 commits into from

Conversation

AaronDewes
Copy link

@AaronDewes AaronDewes commented Mar 18, 2022

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Underlying tools
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

Other information:

This pull request switches to @node-ipc/compat, a fork of node-ipc without malware.

@AaronDewes AaronDewes changed the title fix: switch to @node-ipc/node-ipc fix: switch to @node-ipc/compat Mar 18, 2022
@achrinza
Copy link

achrinza commented Mar 18, 2022

Looks like there's another node-ipc fork as well. :P

Just for reference, I'm also maintaining my own fork for v9 and v10/v11 here: achrinza/node-ipc#1

@@ -1,4 +1,4 @@
const ipc = require('node-ipc')
const ipc = require('@node-ipc/compat')

Choose a reason for hiding this comment

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

Meaby add a little comment explaning why the node ipc has been replace

@@ -1,4 +1,4 @@
const ipc = require('node-ipc')
const ipc = require('@node-ipc/compat')

Choose a reason for hiding this comment

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

Meaby add a little comment here too explaning why the node ipc has been replace

Copy link

@Sacramentix Sacramentix left a comment

Choose a reason for hiding this comment

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

Has a i said in comment, we could add a little comment explaining why the depedency has change.

@DumbGameMaker
Copy link

@node-ipc/compat seems like just a backported version of node-ipc (@node-ipc/node-ipc) not a fork. I could be wrong though. If this is true, might be a better option to choose a third-party fork like achrinza's, because the author of node-ipc could add the patch to @node-ipc/compat

@achrinza
Copy link

achrinza commented Mar 19, 2022

@DumbGameMaker They're all third-party forks of the original package node-ipc with different goals:

  • @achrinza/node-ipc: Maintained by achrinza. Follows upstream closely. Drop-in replacement. No new features.
    • @achrinza/node-ipc@9: Compatible fork of v9
    • @achrinza/node-ipc@10: Compatible fork of v10/v11
  • @node-ipc/*: Maintained by AaronDewes. New features and enhancements such as TypeScript rewrite and use of Promises.
    • @node-ipc/compat@9: Compatible fork of v9
    • @node-ipc/node-ipc@11: Compatible fork of v10/v11

The FAQ provides more info on the difference.

@haoqunjiang
Copy link
Member

Hi, thank you for all the help here!

After reading the FAQ section by @achrinza and reviewing the two projects, I decided to use @achrinza/node-ipc for now. 75826d6

It's not that the other fork isn't good.
But the drop-in replacement approach is easier for me to review:
achrinza/node-ipc@84dce1b...v9
node-ipc/node-ipc@97f4f49...d805d59

And Vue CLI doesn't need any new feature from node-ipc for now, so I've temporarily pinned the dependency version, too.

Thanks again!

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.

5 participants