Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(node, cloudflare): fill in api surface for
node:perf_hooks
#257base: main
Are you sure you want to change the base?
feat(node, cloudflare): fill in api surface for
node:perf_hooks
#257Changes from 1 commit
4104cad
c5345ad
2972049
0bf93fc
9fcf42f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I'm still not sure about this part. Initial injections in nodeless (
global
,process
, andBuffer
) 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(i would suggest to revert this part and split to another PR to unblock)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Difference is:
globalThis.*
natives if available, in Web we doThe 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do have
performance.now()
andperformance.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.