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

Make process requireable #157

Closed
andrewdeandrade opened this issue Dec 13, 2014 · 29 comments
Closed

Make process requireable #157

andrewdeandrade opened this issue Dec 13, 2014 · 29 comments

Comments

@andrewdeandrade
Copy link

Ideally (IMHO) there would be no globals besides the 4 node keywords required for the module system to function: require, module, __dirname and __filename.

For many of the other modules that are typically globals, you can require them out of the box, such as

var console = require('console');
var setTimeout = require('timers').setTimeout; // same with clearTimeout, setInterval and clearInterval
var Buffer = require('buffer');
// etc...

One of the few standard parts of node that cannot be loaded via require() is process.

The popularity of this npm module: https://www.npmjs.com/package/process , with almost a million downloads in the last month shows that a lot of people would benefit from having process be requireable. By making it requireable, browserify can then treat it as one of the special compatibility modules and list it here: https://github.com/substack/node-browserify#compatibility

@ibc
Copy link

ibc commented Dec 13, 2014

+1

@lxe
Copy link

lxe commented Dec 13, 2014

@chrisdickinson
Copy link
Contributor

I don't see any downsides to this, but not any particular upsides either: browserify is likely going to have to check for implicit globals like process for a long time.

I'm +0 on a PR for this -- if one is submitted, and no other TC member has strong feelings about this, I'd be happy to shepherd it.

@bodokaiser
Copy link

+1

@ralphtheninja
Copy link
Contributor

Cool

@buschtoens
Copy link

This would render a few npm packages incompatible. But those could easily and automatically be updated with a PR-bot.

I really like this idea. Globals always felt wrong for me.

What about standard primitives tho?

@andrewdeandrade
Copy link
Author

@buschtoens FWIW the lint rules we use in uber/lint-trap only allow four globals (which I mentioned in my post at the top of this page) whether you are on nodejs or in the browser with browserify. For the browser stuff, we recommend that people use https://github.com/Raynos/global

The great thing about making everything requireable is that it makes it easy for people to mock out fakes when testing since all references in a file point to a closured var that can be overwritten just like substack does with fs here: https://github.com/substack/node-mkdirp/blob/master/test/opts_fs.js#L16

Besides process and global, I think all the other standard primitives are already requireable. For all the timers, you have the timers module. For buffers, the buffer module, etc.

I'm not in favor of implementing this in a way that breaks backwards compatibility (i.e. you should still be able to refer to process as a global, but you can also require it in instead if you prefer). Keeping the standard primitives from being called as a global really is the responsibility of your linter.

@andrewdeandrade
Copy link
Author

@chrisdickinson I've never committed to nodejs or iojs, but I'm open to forking and creating a PR for this if you can just give me a few sentences explaining what parts of the source I should look into to figure out how to go about adding this feature. Just knowing where to start helps a lot.

@brendanashworth
Copy link
Contributor

I have to disagree with a change such as this. In my opinion, it inherently goes against the idea of a module system and a system process.

The built-in modules (such as http, tls, fs, events, util...) are a set of separable, standalone libraries that can be used to build an application on top of a running system process. Should the process global variable be namespaced into a module, it would be categorized as a library (like the other available modules), which is practically a lie to end users. This would turn process into an anti pattern.

The only reason functions like setTimeout and setInterval are attached to global is because they are part of the JavaScript specification. As you stated earlier, you'd like the only four global variables to be the ones required to make the module system function. But do we really need an entire module to tell us the pid? The file descriptor of stdin? Or would it be simpler to use a global variable, not break previous modules, and stay true to the inherent design of a program?

@Raynos
Copy link
Contributor

Raynos commented Dec 17, 2014

We would not remove the global.

Process will always be global. You can just require it as a primary interface. However it has to be global, we cant break back compat.

@brendanashworth
Copy link
Contributor

@Raynos yes, but I still disagree on the basis of it being categorized with the rest of the modules.

@lxe
Copy link

lxe commented Dec 17, 2014

Why not just make require('process') do a noop instead of actually loading a module? You can have the best of both worlds. It's just as much of a hack as making process global in the first place.

Just like Buffer:

require('buffer').Buffer === Buffer // true!

Node io.js is already full of these inconsistencies, and making process behave the same way is a good acceptable idea.

@feross
Copy link
Contributor

feross commented Dec 18, 2014

+1

@seishun
Copy link
Contributor

seishun commented Dec 18, 2014

Can someone explain why this is a good idea? As in, a real-world scenario that would benefit from this?

@andrewdeandrade
Copy link
Author

There are a couple of benefits:

(1) establishes a pointer reference more deeply in the scope chain. when process is required in, any references to process will be resolved in that file instead of going up the scope chain.
(2) makes testing easier. because you have a variable declaration in the file you can expose an option for redeclaring what it points to just like substack does in this test with fs. https://github.com/substack/node-mkdirp/blob/master/test/opts_fs.js . when unit-testing, being able to inject the process.env option you need for that file over the process.env you have in testing.
(3) no more need for requiring a third-party process module when you want process.nextTick available with browserify. Every other native node primitive can be supplied by the module system. Would be nice if you could do the same with process.
(4) some people prefer to declare every token they use in a file, except for the language primitives and keywords.

There was another benefit I thought about this morning, but I can't remember it right now.

@chrisdickinson
Copy link
Contributor

@malandrew a good place to start looking is the merged PR that reintroduced the v8 module -- it should serve as a good example of where to poke at to add a new builtin module. If you need any help, I'm pretty much always available on freenode IRC in #io.js.

@defunctzombie
Copy link
Contributor

As someone who has worked on the node-process module and been involved with the global detection in browserify I am very much +1 for this and making buffer requirable. A v1.0.0 release is a perfect time to remove the buffer global at the very least. It was a mistake to introduce it and not deprecate the global use for this long. Globals themselves are not somehow evil, but core should try to minimize what it leaks into the global scope and lets downstream consumers handle any magic globals they want to inject (if they so desire).

@defunctzombie
Copy link
Contributor

@Raynos even the smallest changes always have a way of breaking backwards compatibility. Saying "we can't break backwards compat" isn't that helpful. Everyone has different understanding of what "compat" is going to mean. And stoping forward progress is the name of "compat" is arguably one of the reasons this repo exists today.

@Raynos
Copy link
Contributor

Raynos commented Dec 21, 2014

@defunctzombie Buffer is different from process.

Buffer does not need to be global. It's just a javascript library.

process is a global thing defined in node.cc and is "special", that being said, I'm ok with it not being global.

Also note being able to require process is more important then removing globals.

@andrewdeandrade
Copy link
Author

+1 for deprecating global Buffer (but that's an topic for another github issue).

Agree that process is different in the sense that it is a singleton object. It's a singleton that definitely should be available via the module system (hence why I opened this ticket). Whether or not it's use as a global should be deprecated, that is a topic for a different issue after a PR resolving this issue has been submitted and merged.

I'll try to get this PR done over the christmas holiday.

@seishun
Copy link
Contributor

seishun commented Dec 23, 2014

(1) establishes a pointer reference more deeply in the scope chain. when process is required in, any references to process will be resolved in that file instead of going up the scope chain.

The scope doesn't go beyond the file so I'm not sure what you mean. What's a "pointer reference"? Can you give an example?

(2) makes testing easier. because you have a variable declaration in the file you can expose an option for redeclaring what it points to just like substack does in this test with fs. https://github.com/substack/node-mkdirp/blob/master/test/opts_fs.js . when unit-testing, being able to inject the process.env option you need for that file over the process.env you have in testing.

That only works if the module explicitly uses require('process') instead of process, i.e. has testing in mind. What prevents it from doing something like var proc = process then?

(3) no more need for requiring a third-party process module when you want process.nextTick available with browserify. Every other native node primitive can be supplied by the module system. Would be nice if you could do the same with process.

The nextTick in node-process is basically just setTimeout(fun, 0) with a queue. If people writing for browsers really need it, why can't browserify just provide require('process') now? I really don't see why node/io needs to be affected.

(4) some people prefer to declare every token they use in a file, except for the language primitives and keywords.

They can always do var process = process;.

+1 for deprecating global Buffer (but that's an topic for another github issue).

Buffer is such a ubiquitous thing that it deserves to be global. Removing it would break a lot of code for literally zero gain.

@defunctzombie
Copy link
Contributor

Buffer is such a ubiquitous thing that it deserves to be global. Removing it would break a lot of code for literally zero gain.

No, it really isn't. If it was a module from day 1 you would not think twice about it. I think it is a reasonable policy to avoid introducing more globals than what is already part of the language spec.

@fbender
Copy link

fbender commented Dec 27, 2014

If I'm not completely mistaken, process is a host object and the global (equivalent to window in browsers) and making it requirable is not sensible. However, I welcome making require('process') a no-op which Browserify can leverage but does not really change anything within io.js.

No, it really isn't. If it was a module from day 1 you would not think twice about it.

That's a non-sequitur. We cannot change the past, and since Buffer is a global now this will stick with us forever unless we brake a sh*tload of existing code. I certainly believe the decision to have Buffer as a global was correct since I can't remember any non-trivial code that does not use buffers in one or the other form (it's really the basis of any I/O or stream-processing functionality which is like 90% of the API).

I think it is a reasonable policy to avoid introducing more globals than what is already part of the language spec.

There's a difference between not adding more globals vs. removing one. I agree with the first but am hesitant with the second. Though adding more globals than defined in the spec is not an issue per se and practiced every day within the DOM – we can discuss whether the way they do it is sensible, but this is out of scope of this bug (as well as making Buffer requirable).

@alexpods
Copy link

@fbender You're wrong. process is not global object like window in browser. When you're adding property to process it's not becoming a global variable (becoming accessible from any module). Global object in node is global.

process.something = 10;
console.log(something); // undefined or ReferenceError: something is not defined

global.something2 = 20;
console.log(something2); // 20

But, in my opinion module is something that is not dependent on current execution script. For example when you require('underscore') you always receive exactly the same underscore module, no matter from where you require it. But process is completely bound to current execution. You will get different properties process.env, process.pid (and so on) from different executions. buffer on the other hand is independent from current execution.

So my humble opinion:

  • process must be global (and not deprecated), but also can be requirable
  • buffer must be requirable, global buffer must be deprecated

@brendanashworth
Copy link
Contributor

I think that, if browserify or libraries like that want process to be requireable, I'll support this, but only as long as the process global isn't deprecated, so 👍 to what @alexpods is saying.

@lxe
Copy link

lxe commented Dec 29, 2014

From my limited understanding, process is a global object exposed and passed around in node.cc as well as node.js and other places. A lot of library code probably depends on it being global, and changing it might require a significant refactor.

This isn't about removing process from the global scope. I don't think that process should stop being accessible globally, even if it could be done.

It should merely re-exported to be require()able to adhere to convention (and make linting easier). Even in the case of Buffer, it's accessible both globally, and through a require(), which is how process should behave as well.

@reqshark
Copy link

well plus oneing this issue now is getting weird because it is saying

Ideally (IMHO) there would be no globals besides the 4 node keywords required for the module system to function: require, module, __dirname and __filename.

echoing some of @lxe, i think it sounds like a cool idea but all of my code will break if I dont go

var process = require('process')

at the top.

Process is different, I have to agree with @brendanashworth's first remark.

Process is to ios.js as the window object is to the browser.

👍 making it a require.

👎 making it not a global.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 2, 2015

Closing as #206 was landed.

@nodejs-github-bot

This comment has been minimized.

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

No branches or pull requests