defunctzombie / node-process

process information for node.js and browsers
MIT License
122 stars 62 forks source link

errors are not necessary in some cases #74

Open ORESoftware opened 7 years ago

ORESoftware commented 7 years ago

I think there is a specification error in the code

Uncaught Error: process.binding is not supported
    at Object.process.binding (suman.js:293)
    at Object.<anonymous> (suman.js:51174)
    at Object.module.exports.100 (suman.js:51178)
    at __webpack_require__ (suman.js:20)
    at Object.<anonymous> (suman.js:3417)
    at __webpack_require__ (suman.js:20)
    at Object.<anonymous> (suman.js:38562)
    at Object.<anonymous> (suman.js:39238)
    at __webpack_require__ (suman.js:20)
    at Object.<anonymous> (suman.js:80743)

It would be much better if process.binding() and process.chdir() were no-ops. Throwing an error literally just breaks things, there is no point in polyfilling functions and just throwing an error, it doesn't make any sense. If the behavior is not supported in the browser just make it a noop. I hope this makes sense to you!

As you can see, in your existing code process.cwd() is pretty much a no-op even though it's not supported in the browser really either.


process.binding = function (name) {
    throw new Error('process.binding is not supported');  //does nobody any favors
};

process.cwd = function () { return '/' };

process.chdir = function (dir) {
    throw new Error('process.chdir is not supported');  // does nobody any favors
};
ORESoftware commented 7 years ago

Specifically, I see process.binding()being called here:

  /***/ (function(module, exports, __webpack_require__) {

    "use strict";
    /* WEBPACK VAR INJECTION */(function(process) {

      var blacklist = [
        'freelist',
        'sys'
      ];

      module.exports = Object.keys(process.binding('natives')).filter(function (el) {
        return !/^_|^internal|\//.test(el) && blacklist.indexOf(el) === -1;
      }).sort();

      /* WEBPACK VAR INJECTION */}.call(exports, __webpack_require__(1)))

    /***/ }),
calvinmetcalf commented 7 years ago

I think the idea here is that process.binding is something that ONLY applies to node, not to browser code so that if it is ever called in the library you are almost certainly attempting to use a module that won't work and thus should raise an error.

What is the module that raising the errors ?

ORESoftware commented 7 years ago

It's not my code, I cannot control it, it might be a core module, or an NPM module, not sure which. I can check. But it does come from the code I pasted above.

ORESoftware commented 7 years ago

@calvinmetcalf if you think about it, the developer using your library can carry the responsibility of checking if the code is running in the browser or node.js, or wherever.

the developer can call:

if(isNode){
  process.binding() // etc etc
}
else{
  // we are in browser
}

throwing an error doesn't help anyone - it gives the developer using your polyfill less power/less control.

The developer using your library is smart enough to not call process.binding()if they don't want to call it :)

So with that in mind, for code that I cannot control, it would be best to mimic Node.js as much as possible.

calvinmetcalf commented 7 years ago

You are trying to use in a browser a module called builtin-modules, you 100% do not want to bundle this up for use in the browser as it's just a list of of the paths on your hard drive where you can find the builtin-modules.

ORESoftware commented 7 years ago

@calvinmetcalf I might be be able to avoid bundling builtin-modules, but right now the call to builtin-modules is in my codebase and will get bundled by webpack.

ultimately, my problem will be solved if you were to accept the PR changes. I am not sure if it would cause others problems.

ORESoftware commented 7 years ago

@calvinmetcalf my use case is different, this is not for a web application. I am trying to get a library that was heavily written for backend use, into the browser. Unfortunately, I can't really rewrite my library to make it more browser friendly. For my use case, the better way to do it is to shim things so that they are at worst no-ops in the front-end.

I might just have to steal the code and adapt it for my use, that's no big deal.

From a philosophical standpoint however, I strongly believe it makes no sense to throw errors for functionality that does not exist in the front-end. Just make it a noop. It literally helps nobody to throw an error. IMO.

calvinmetcalf commented 7 years ago

ok so my reasoning is that most every situation I can think of code that accessed bindings is an error that you didn't mean to include and having an early error helps because it lets you know oops I didn't mean to include this file in my bundle.

making a fork that does something unique is something I've done myself, you could probably also just monkey patch this library.

out of curiosity what exactly are you trying to bundle, there may be a significantly easier way to do what you are trying to do.

ORESoftware commented 7 years ago

I am trying to bundle a test library - https://github.com/sumanjs. It's similar to AVA, it's heavily backend (node.js) oriented, but I know that it can be lifted into the browser with the right polyfills, but I may have to write most of those polyfills myself. For example, there apparently is no good fs polyfill, but I know that I can write one that will fit my use case.

Fortunately, people using my test library in the browser, probably won't need the same polyfills, so they won't clash.

I am not a big believer in unit tests that run in the browser, now that many cross-browser incompatibilities no longer exist as much. So I wrote Suman to be backend oriented - the best way to test browser is to use Selenium etc, instead of unit tests that run in the browser.

But now I realize it would be a nicety for suman to be able to run in the browser, so here I am trying to retrofit it a bit. Instead of rewriting a lot of code, better to just polyfill/shim backend utilities such as fs. It's a pretty fun task to try to browserify the damn thing actually.

calvinmetcalf commented 7 years ago

https://github.com/mafintosh/browserify-fs

On Wed, Apr 5, 2017 at 5:07 PM Operations Research Engineering Software+ < notifications@github.com> wrote:

I am trying to bundle a test library - https://github.com/sumanjs. It's similar to AVA, it's heavily backend (node.js) oriented, but I know that it can be lifted into the browser with the right polyfills, but I may have to write most of those polyfills myself. For example, there apparently is no good fs polyfill, but I know that I can write one that will fit my use case.

Fortunately, people using my test library in the browser, probably won't need the same polyfills, so they won't clash.

I am not a big believer in unit tests that run in the browser, now that many cross-browser incompatibilities no longer exist as much. So I wrote Suman to be backend oriented - the best way to test browser is to use Selenium etc, instead of unit tests that run in the browser.

But now I realize it would be a nicety for suman to be able to run in the browser, so here I am trying to retrofit it a bit. Instead of rewriting a lot of code, better to just polyfill/shim backend utilities such as fs. It's a pretty fun task to try to browserify the damn thing actually.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/defunctzombie/node-process/issues/74#issuecomment-291997505, or mute the thread https://github.com/notifications/unsubscribe-auth/ABE4n-SED-2vMByjX7WOvlht5jxVxkahks5rtAKtgaJpZM4MzzX0 .

ORESoftware commented 7 years ago

@calvinmetcalf I tried that I believe, I don't remember it working for me, but will take another look though

ORESoftware commented 7 years ago

What I actually will want to do is patch fs for the browser and make most of the methods on fs no-ops. Looks like browserify-fs is attempting to fill in some of the functionality of fs, but I don't need that.

ORESoftware commented 7 years ago

@calvinmetcalf this is where I could use your help though -

https://github.com/webpack/webpack/issues/4640

if you look towards the bottom, what I am looking to do is use Webpack and plug-in my own polyfills instead of using the "official" ones. If you have any idea how to do that, please lmk.