feat(node, cloudflare): fill in api surface for node:perf_hooks
#257
Conversation
- added missing APIs - created a hybrid polyfill for cloudflare - updated injectable config for globalThis.performance
pi0
changed the title
node:perf_hooks
|
@pi0 I believe this PR is good to go. PTAL |
| "perf_hooks", | ||
| "PerformanceObserverEntryList", | ||
| ], | ||
| PerformanceResourceTiming: ["perf_hooks", "PerformanceResourceTiming"], |
There was a problem hiding this comment.
To be honest, I'm still not sure about this part. Initial injections in nodeless (global, process, and Buffer) are all Node.js specific global which were safe to inject globally. Performance API is a Web standard on the other hand. By adding this injections we are effectively overriding any native global implementation in user-code to unenv/node.
I think this should be an explicit opt-in. wrangler preset (#262) is a good candidate because i suppose you also require explicit (Node-compat) from users to enable this and then things make sense.
There was a problem hiding this comment.
(i would suggest to revert this part and split to another PR to unblock)
There was a problem hiding this comment.
Performance API is a Web standard on the other hand. By adding this injections we are effectively overriding any native global implementation in user-code to unenv/node.
You make a really good point, Pooya! It's funny how we sometimes get caught up in not seeing the obvious.
I think the right thing here is to move these injects into the cloudflare preset because workerd doesn't offer Performance* APIs.
I'm not yet convinced that we need a dedicated wrangler preset. I'm hoping we really don't have to do that.
Would you be ok with me updating the PR by moving these injects into preset/cloudflare?
There was a problem hiding this comment.
I want to use same cloudflare preset in Nitro builds as well and users would face similar implicit opt-in to Node.js variant of Performance* variants if we do this for them (inject Web vs Node version)
There was a problem hiding this comment.
I see what you mean. The issue you are bringing up is that Node's Performance* API is not 100% compatible with Web's Performance* and if someone relies on globals we'd be making the choice for them, which might not be good. This is quite messy. How big is the difference between the two?
@jasnell as a node core contributor, have you seen this become a problem in the node community? how do people deal with these compat issues?
There was a problem hiding this comment.
- Current web impl: https://github.com/unjs/unenv/tree/main/src/runtime/web/performance
- Current node impl (extending web): https://github.com/unjs/unenv/blob/main/src/runtime/node/perf_hooks/internal/performance.ts
Difference is:
- In node impl, we always implement class and won't fall back to
globalThis.*natives if available, in Web we do - Some classes only allow constructor via polyfills and not with natives and we need it internally between polyfills
- Node.js classes have additional props
The first is solvable somehow but I'm more worried about the design decisions we hare making for unenv. For me unless explicitly we are adding "node.js" compatibility, we should strictly stick with Web standards spec. Only for wrangler usage that uses unenv as Node.js compatibility layer it clearly makes sense but for generic polyfill, it breaks the concept.
I am thinking to expose new API from unenv that we can have flags like nodeCompat to make difference behavior.
In the meantime, let's move these to cloudflare as you suggested (we can have web for nodeless + node for cloudflare that overrides nodeless) 👍🏼 this PR seems unnecessarily being blocked.
There was a problem hiding this comment.
we do have performance.now() and performance.timeOrigin in workers. The impl in Node.js definitely has a bunch of Node.js specific stuff. It hasn't been too much of a problem in the ecosystem but it definitely warrants caution.
|
@pi0 I'll get back to this one within a few days. I still care, just busy with other things. |
|
No rush, thanks for the update @IgorMinar |
Reference: #