node v21.0.0 breaks with xterm: Cannot read properties of undefined (reading 'includes')

[davidfiala]

node v21.0.0 introduces the navigator object where it used to be undefined.

Which causes issues in src/common/Platform.ts:

export const isNode = (typeof navigator === 'undefined') ? true : false;
const userAgent = (isNode) ? 'node' : navigator.userAgent;
const platform = (isNode) ? 'node' : navigator.platform;

export const isFirefox = userAgent.includes('Firefox');

There was discussion on whether using navigator to detect node here: https://github.com/nodejs/node/pull/47769#issuecomment-1601054678 but ultimately they concluded that since Deno has introduced a navigator object, so can node.

I often see checks like this instead:

const isNode = (typeof process !== 'undefined');
// or
const isNode = (typeof global !== 'undefined');

which will work for v21 and below.

[davidfiala]

Some recent updates on the node side: As more projects have noticed breakages introduced by adding navigator in v21.0.0, the node team recently merged in navigator.userAgent in v21.1.0.

If you'd like, I could add another PR to detect that, but I'd also have to account for a few other cases which will make it look a bit weird for the next few years.

While my PR above with const isNode = (typeof process !== 'undefined'); works for all versions AFAIK, it's always possible this may break again in the future.

Detection edge cases we'd need to support for a while:

• < v21.0.0 no process object, no navigator object
• == v21.0.0 no process object, yes navigator, no navigator.userAgent
• >= v21.1.0 no process object, yes navigator, yes navigator.userAgent
• future process object could change?
• another 3p library Or what if another library creates a global process object, thus confusing xterm's detection logic

It's certainly fine to do nothing and keep things as are, as I doubt any change will be rapid. My offer would be to provide a PR to add a back-stop so that if navigator.userAgent exists, we'd string match for Node.js. And likewise, we wouldn't assume that the existence a of global process implies node unless there also is no navigator.userAgent present. LMK if you'd like this change.

Refs:

• More discussion on adding navigator.userAgent: https://github.com/nodejs/node/pull/50200
• v21.1.0 code change to add useragent, including the string to check: https://github.com/nodejs/node/commit/5a52c518ef

[Tyriar]

@davidfiala thanks for looking into this, ideally we'd like whatever is the most future proof if you want to contribute an improvement 🙂