browser does not have global defined. Should this be globalThis?

[NullVoxPopuli]

Description

I want to load my tests. :)

Issue

Environment

• node -v output: Chrome (no node)
• npm -v (or yarn --version) output: (doesn't matter?)
• npm ls testdouble (or yarn list testdouble) version: [email protected]

I don't yet have time to do any digging, so I'm skiping the repro stuff below.
Just wanted to report this for now in case something obvious is happening.

I've been able to get passed this error with:

    <script>
      window.global = window;
    </script>

but is this a hack? is this build-environment-dependent? idk

Failing Test

• Fork the repo
• Add a failing test (probably to the `/regression/src' directory)
• Submit a pull request for the failing test or link to your branch here

Example Repo

• Create a minimal repository that reproduces the issue
• Make sure that a fresh clone can run only npm it and observe the issue
• Link to that repo here

Runkit Notebook

• Create a Runkit notebook
• Invoke var td = require('testdouble') at the top
• Verify the behavior your issue is concerned with by clicking "Run"
• Link to the Runkit here

Code-fenced Examples

var td = require('testdouble')

// Your steps here.

[searls]

I spent a few hours on this and simply can't replicate it. Could you please provide a repo with which to reproduce this?

[NullVoxPopuli]

How'd you try to reproduce it?

Just in the console, I reproduce the lack of global

[searls]

No I mean how are you installing and building your application such that you're seeing this behavior

[NullVoxPopuli]

Sorry for my lack of understanding :( , but I don't understand the relevance? Do different environments polyfill different globals?

But I'm installing with yarn, and bundling with webpack (specifically for the browser which lacks 'global', used in these places: https://github.com/testdouble/testdouble.js/search?utf8=%E2%9C%93&q=global&type= )

And in eval space here:

testdouble.js/src/config.js

Line 7 in cacc21f

promiseConstructor: global.Promise,

[NullVoxPopuli]

To reproduce:

Clone repo and delete this line: https://github.com/NullVoxPopuli/emberclear/blob/master/packages/frontend/tests/index.html#L37

error will occur during test run (I forget which one atm): cd packages/frontend && yarn && yarn ember test

[searls]

Thank you. The reason for asking is because I've checked half a dozen other projects building with webpack and none of them exhibit this behavior, even when using the promise feature you pointed to:

[searls]

I pulled down your repo (update: and deleted the line from your test index file), then ran cd packages/frontend && yarn && yarn ember test but can't reproduce.

1..260
# tests 260
# pass  246
# skip  14
# fail  0

There's no mention of testdouble or td in the output.

[NullVoxPopuli]

Ah, it looks like I removed it from my tests: https://github.com/NullVoxPopuli/emberclear/search?utf8=%E2%9C%93&q=testdouble&type=
Sorry for making you pull the code down.

I'm super new to test double 😭 been thinking about learning it as an alternative to all of the sinon APIs.

none of them exhibit this behavior, even when using the promise feature you pointed to:

What environment is that? Does it define 'global'?

Fwiw, I think by default, webpack polyfills a bunch of node stuff so you don't run in to build errors -- I wonder if 'global' is a node thing?

Confirm!
Node has global: https://nodejs.org/api/globals.html#globals_global

So, I really don't like this behavior of webpack, cause.. while it makes for a better newbie experience when configuring webpack, and pulling in random npm packages (not noticing if intended for the browser or node), and making things 'just work'... But like.. you'd get 'fs' , 'path', 'crypto', etc. This super bloats your bundle.

After checking the node documentation, idk what you'd do as a maintainer to support pure browser usage. The 'global' need to be abstracted or something.. like..

Idk, I found this:

 var getGlobal = function () {
	// the only reliable means to get the global object is
	// `Function('return this')()`
	// However, this causes CSP violations in Chrome apps.
	if (typeof self !== 'undefined') { return self; }
	if (typeof window !== 'undefined') { return window; } // same as globalThis
	if (typeof global !== 'undefined') { return global; }
	throw new Error('unable to locate global object');
};

https://github.com/tc39/proposal-global/blob/master/README.md

[searls]

Yes, that's correct. global is the name of the Node.js global.

Even if this one line were obviated, I am not confident that the library would actually work when loaded this way by a browser.

For browser use, the package includes a browserify build of the library in dist/testdouble.js and which should be able to be loaded with a td global pre-defined. (edit: as mentioned in the README, this can also be loaded via unpkg: https://unpkg.com/[email protected]/dist/testdouble.js )

[NullVoxPopuli]

hmmm. gotchya

now that I'm at a computer, I'm gonna get you something more concrete

[NullVoxPopuli]

reproduction: https://github.com/NullVoxPopuli/testdouble-global-missing-reproduction
error:

ReferenceError: global is not defined
                at eval (webpack://__ember_auto_import__/./node_modules/testdouble/lib/config.js?:15:23)
                at Object../node_modules/testdouble/lib/config.js (http://localhost:7357/assets/test-support.js:18914:1)
                at __webpack_require__ (http://localhost:7357/assets/test-support.js:19535:30)
                at eval (webpack://__ember_auto_import__/./node_modules/testdouble/lib/store/stubbings.js?:23:16)
                at Object../node_modules/testdouble/lib/store/stubbings.js (http://localhost:7357/assets/test-support.js:19334:1)
                at __webpack_require__ (http://localhost:7357/assets/test-support.js:19535:30)
                at eval (webpack://__ember_auto_import__/./node_modules/testdouble/lib/function.js?:13:19)
                at Object../node_modules/testdouble/lib/function.js (http://localhost:7357/assets/test-support.js:18950:1)
                at __webpack_require__ (http://localhost:7357/assets/test-support.js:19535:30)
                at eval (webpack://__ember_auto_import__/./node_modules/testdouble/lib/index.js?:7:18)
        message: >

[NullVoxPopuli]

I found the file you're talking about, and how global would be abstracted away

but I can't find out how this file would get included?
in package.json, the main points at lib/index.js
and the browser doesn't point at dist/testdouble.js either

 "browser": {
    "./src/replace/module/index.js": "./src/replace/module/index.browser.js",
    "./lib/replace/module/index.js": "./lib/replace/module/index.browser.js",
    "quibble": "./src/quibble.browser.js"
  },

am I missing something really obvious? haha

[searls]

cc/ @jasonkarns in this year of 2020, is there yet an agreed up on correct way to indicate a browser distribution in package.json?

I imagine require('testdouble/dist/testdouble') or import 'testdouble/dist/testdouble'? Else you could just add its relative path via node_modules to your tests/index.html file in a script tag as you are doing with other assets

[NullVoxPopuli]

I think it is supposed to be the browser field https://docs.npmjs.com/files/package.json#browser

I've never made a multi-platform npm package before, so.. I'm not much help anyway, but should dist/testdouble.js be listed in there?

[searls]

Yes, that's been npm's documentation for a long time IIRC but there's been a lot of back and forth on actual best practice around that field and of course, predating any of it, browserify uses the field as a way to swap out browser-incompatible node files (as we are doing above)

[NullVoxPopuli]

I had thought brawserify was dead, but after checking out their GitHub, It is not! It looks like, based on their goals of using node things in the browser that it's not possible to support brawserify and modern web dev. :/

Turns out, they have an open issue about this: browserify/browserify#1694

[NullVoxPopuli]

Though, even after reading that issue, it seems the brawserify folks actually do use 'browser' as an alternate entry point?

[jasonkarns]

@searls Sorry, I've been way out of this space for too long to have any valuable input here.

[searls]

@NullVoxPopuli I read the thread and agree that this is an issue. We are using the browser field currently so that people can use the library in a CJS compatible format when targeting browsers by duck-punching/replacing specific browser-incompatible files (as discussed in the spec). Browserify itself also looks at that key to successfully build the package without including sources that'll explode on impact with a browser JS environment.

So, because we're already using the key for that purpose, we can't also use it as a way to signal where the generated/gitignored bundle is to potential clients. As a result I don't think there's yet another to signal to tools "hey this is where the fully bundled JS distribution is with a global set on window, etc".

I'm going to close this for now, but if at some point someone submits a PR that makes this better for everyone without breaking anything, I'd absolutely welcome it!