vikejs / vike-node

🔨 Node integration for Vike
MIT License
23 stars 3 forks source link

refactor: replace try-import with globalThis.process #18

Closed rtritto closed 1 month ago

brillout commented 1 month ago

Isn't checking after node:http more precise and thus better?

rtritto commented 1 month ago

Because the node:http is an hack while globalThis is more cross-platform (window (Browser) + self (Web Worker) + global (Node.js)) and more consistent.

brillout commented 1 month ago

Precisely, ins't checking after node:http more accurate? What we want here is to check whether the environment supports connect handlers which is only supported in Node.js. For that purpose checking after node:http seems a lot more accurate, right?

rtritto commented 1 month ago

What we want here is to check whether the environment supports connect handlers which is only supported in Node.js

You can simply check if you are in the Node.js environment.

Some points to prefer the globalThis approach:

Absurdly, the user can install a library called node:http.

brillout commented 1 month ago

The issue with globalThis.process !== undefined is that it isn't reliable. For example, any library could be defining globalThis.process and consequently break vike-node. So I still think checking whether node:http exits to be more reliable. Closing, but let's see if we stumble upon a real world issue with the node:http check and I'll then re-consider this.

rtritto commented 1 month ago

As alternative you can enforce with:

export function isNodeLike() {
  return globalThis.process !== undefined && globalThis.process.versions?.node !== undefined
}

Some source: https://www.30secondsofcode.org/js/s/is-node-or-browser

brillout commented 1 month ago

I ain't sure it catches Bun or Deno then.

rtritto commented 3 weeks ago

I ain't sure it catches Bun or Deno then.

We can use instead:

export function isNodeLike() {
  return (
    globalThis.window === undefined &&
    globalThis.document === undefined &&
    globalThis.navigator === undefined
  )
}
brillout commented 3 weeks ago

I think it's better we check for capability instead.