BrowserDetector detects Node.js v21.0.0 and later as being a browser

[trentm]

What happened?

Steps to Reproduce

$ nvm use 21    # switch to Node.js v21 somehow

$ cd packages/opentelemetry-resources
$ npm test

Expected Result

Tests should pass.

Actual Result

They fail one test:

$ npm test
...
  Node.js: browserDetector()
    1) should return empty resources if window.document is missing
...
  56 passing (153ms)
  10 pending
  1 failing

  1) Node.js: browserDetector()
       should return empty resources if window.document is missing:

      AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

3 !== 0

      + expected - actual

      -3
      +0

      at assertEmptyResource (test/util/resource-assertions.ts:375:10)
      at Context.<anonymous> (test/detectors/node/BrowserDetector.test.ts:25:24)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

Additional Details

BrowserDetectorSync is using const isBrowser = typeof navigator !== 'undefined';

opentelemetry-js/packages/opentelemetry-resources/src/detectors/BrowserDetectorSync.ts

Line 27 in aabd1a9

const isBrowser = typeof navigator !== 'undefined';

but Node.js v21.0.0 added a global navigator:
https://nodejs.org/api/all.html#all_globals_navigator

[trentm]

Thoughts on using window !== undefined?

Both latest Node.js and Bun have a navigator global now, FWIW.

@MSNev Any experience or opinions here for how to identify if we are in a browser runtime?

[MSNev]

While window won't exist on node, so that would work. It also doesn't exist in a web worker either so that would still be a browser scenario.

I generally, go the other way around and detect whether the environment is node, see the "helpers" I have in the "Runtime Environment Checks" section of this https://github.com/nevware21/ts-utils?tab=readme-ov-file#documentation-and-details.

So basically, if something "wants" to use window it uses either getWindow() or hasWindow(), which are all wrapped in try / catch so that environments that don't like you trying to access globals that don't exist don't cause a failure.

And likewise for the getDocument() / hasDocument(), again workers generally don't have access to the DOM.

[dyladan]

I think testing node environment variables should be possible to detect if we're in node. Something like undefined != process?.env?.["NODE_VERSION"] maybe