Reduce our patches for node

Question

Reduce our patches for node

zcbenz opened this issue 11 years ago · 17 comments

Currently we have about 20 commits based on upstream node's head, we should keep our patches as minimum as possible and try to get some of them into upstream.

Answer 1 · 2013-12-20T05:45:55.000Z

Tried to merge one patch to upstream: nodejs/node-v0.x-archive#6744.

Answer 2 · 2013-12-20T18:38:22.000Z

Nice. Glad you're doing this. Looks like they're open to it?

Answer 3 · 2013-12-21T17:00:38.000Z

Yeah they are not against it, but it's not easy to persuade them to accept it too.

Answer 4 · 2014-06-24T17:54:05.000Z

Any updates on this @zcbenz?

Answer 5 · 2014-06-25T04:29:36.000Z

No news yet, but I have added more information in that PR since atom had been publicized, hoping it could add chances to get our patches merged.

Answer 6 · 2014-08-15T05:12:22.000Z

A small patch merged to upstream: nodejs/node-v0.x-archive#8122.

Answer 7 · 2014-08-15T17:19:56.000Z

Nice work @zcbenz ✨

Answer 8 · 2015-01-07T22:58:03.000Z

Another patch merged to io.js.

Answer 9 · 2015-01-20T23:24:47.000Z

Another one merged.

Answer 10 · 2015-06-30T17:53:15.000Z

@zcbenz is there a way we (io.js / node) can better support you in this? We'd like to help reduce the number floating patches required for embedders, if possible.

Answer 11 · 2015-06-30T18:02:28.000Z

It looks like a good amount of the patches are actually on libuv. Have you thought about submitting them to https://github.com/libuv/libuv?

Answer 12 · 2015-07-05T09:49:38.000Z

@Fishrock123 For simple patches I usually just create a pull request to the repo, like libuv/libuv#395, but most of our patches are dirty hacks, it takes a lot of efforts to make them decent APIs with test cases, which I don't have time to do now. But eventually when I got time I would love to merge them to upstream.

Answer 13 · 2015-07-07T18:52:34.000Z

Right, but don't be afraid to open issues for some things too! We do need to know what is necessary to make embedders' lives better. :)

Answer 14 · 2017-09-28T15:07:20.000Z

Hello all,
If you need someone to help port stuff to mainline node, feel free to ping me.

but most of our patches are dirty hacks,

I'm quite sure that beside me there are other node contributors that would be happy to help turn hacks into APIs.

And RE: dc8fe9d there's a PR open to make those opt-in nodejs/node#15454

Answer 15 · 2017-10-02T05:41:24.000Z

@refack That would be great help!

Answer 16 · 2017-10-02T14:44:03.000Z

@refack That would be great help!

I'll take a look at the diff. If you could prioritize stuff, that would help me help you ;)

Answer 17 · 2017-10-03T11:35:21.000Z

@refack I'm going to go through the patches in this issue.

Hide console window when creating process.
e6afee6

Generally we should have an option in libuv and Node.js for this flag, but it is useless for console apps and in Electron (and other embedders) we definitely want to have it on by default.

So we can probably add a build flag in libuv to turn it on by default.

Make Module.globalPaths a reference.
f4ad6b7

We need this for security, so desktop apps won't accidentally load a module installed on user's system.

Maybe adding adding a new flag like what we did with process._noBrowserGlobals?

Don't set wrapper class in node, otherwise heap snapshot would crash.
6a24dd7

We commented that code out because it conflicts with blink web engine, no idea how to do with it.

Guard against NULL returned by uv_err_name.
124a0f2

Some antivirus and firewall softwares can block socket connections from other software clients, on our side we would get an unnormal errorno which would make uv_err_name return nullptr. I think the biggest challenge is how to represent an unknown error.