disable built-ins in Browserify

[mshaaban0]

Purpose (TL;DR) - mandatory

In favor of @mantoni's comment in sinonjs/commons#37 (comment)

I want to raise awareness for the fact that browserify installs shims for things like process if they're used in the codebase. Maybe we can configure it to not do that. Libraries or apps using Sinon might use typeof process === "object" as Node environment detection.

How to verify - mandatory

1. Check out this branch
2. npm install
3. npm run build

Checklist for author

• npm run lint passes

[fatso83]

I guess the verification step would be something like concatenating this script with a simple check for window.process and run that in puppeteer.

[mantoni]

I've looked into this and already did some work in sinonjs/fake-timers#270 and sinonjs/nise#108. I think the flag we want is detectGlobals: false. We could also set builtins: false, not sure if that actually changes anything.

The next step is to integrate the above changes in Sinon and make sure tests pass in all supported environments with these settings.

[fearphage]

I guess the verification step would be something like concatenating this script with a simple check for window.process and run that in puppeteer.

What are we verifying? Do we need to test that browserify options work?

[fatso83]

It's an integration test. We test that the individual bits have been assembled correctly and works as expected in unison. So yes. It's just a simple smoke test, not anything involved.

[mantoni]

Like this one in Mochify I guess? https://github.com/mantoni/mochify.js/blob/master/test/chromium-test.js#L475

[mshaaban0]

--no-builtins removes browserify's node native modules mapping. Shall I add detectGlobals: false to all bundles?

[fatso83]

I'm not sure what that actually means/does ...

[fatso83]

OK, so I looked into this and why this fails. Basically, disabling built-ins means we have to do the work of finding or injecting these globals ourselves, like in the good old days.

So the above build triggered by @mshaaban0 failed due to neither window nor global being defined in a WebWorker. I had to change the next-tick.js to defensively check for window, then global, then self.

After that, it failed in Lolex, which also has some global || window tricks, which fails. After trying to fix those, it was Nise's turn, as it also uses global quite a bit.

Not totally sure what the best solution ™️ is here, but a relatively simple one could be to add getGlobalObject() to @sinonjs/common and use that in every Sinon project to avoid the issue.

function getGlobalObject(){
  var root;
  if (typeof window !== "undefined") {
    root = window;
  } else if (typeof global !== "undefined") {
    root = global;
  } else if (typeof self !== "undefined") {
    root = self;
  }
  return root;
}

Once the dependencies have been patched to handle not injecting process, we could finish this.

But all of this seems a bit unnecessary ... I was hoping that all these globals were just defined by an outer closure per module and not for the entire bundle. That would have made these issues local.

[mantoni]

This has been fixed with #2175. Closing.