Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

rtritto
Copy link
Contributor

@rtritto rtritto commented Oct 19, 2024

No description provided.

@brillout
Copy link
Member

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

@rtritto
Copy link
Contributor Author

rtritto commented Nov 1, 2024

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
Copy link
Member

brillout commented Nov 1, 2024

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
Copy link
Contributor Author

rtritto commented Nov 1, 2024

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:

  • it's not hacky
  • it's not an async/await function
  • gains in performance
  • uses native check related to the environment

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

@brillout
Copy link
Member

brillout commented Nov 1, 2024

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.

@brillout brillout closed this Nov 1, 2024
@rtritto
Copy link
Contributor Author

rtritto commented Nov 1, 2024

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
Copy link
Member

brillout commented Nov 1, 2024

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

@rtritto
Copy link
Contributor Author

rtritto commented Nov 4, 2024

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
Copy link
Member

brillout commented Nov 4, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants