-
Notifications
You must be signed in to change notification settings - Fork 22
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
#257
Merged
Merged
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
4104cad
feat(node, cloudflare): fill in API surface for node:perf_hooks
IgorMinar c5345ad
fixup! feat(node, cloudflare): fill in API surface for node:perf_hooks
IgorMinar 2972049
fixup! feat(node, cloudflare): fill in API surface for node:perf_hooks
IgorMinar 0bf93fc
fixup! feat(node, cloudflare): fill in API surface for node:perf_hooks
IgorMinar 9fcf42f
fixup! feat(node, cloudflare): fill in API surface for node:perf_hooks
IgorMinar d4cbd23
Merge branch 'main' into pr/IgorMinar/257
pi0 3a6d911
refactor: move performance injections to cloudflare
pi0 60d7067
revert unwanted change
pi0 29105fc
fix type issues
pi0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
import type nodePerfHooks from "node:perf_hooks"; | ||
|
||
export { | ||
Performance, | ||
PerformanceEntry, | ||
PerformanceMark, | ||
PerformanceMeasure, | ||
PerformanceObserverEntryList, | ||
PerformanceObserver, | ||
PerformanceResourceTiming, | ||
constants, | ||
createHistogram, | ||
monitorEventLoopDelay, | ||
} from "./index"; | ||
|
||
import { | ||
Performance, | ||
PerformanceEntry, | ||
PerformanceMark, | ||
PerformanceMeasure, | ||
PerformanceObserverEntryList, | ||
PerformanceObserver, | ||
PerformanceResourceTiming, | ||
constants, | ||
createHistogram, | ||
monitorEventLoopDelay, | ||
performance as unenvPerformance, | ||
} from "./index"; | ||
|
||
// The following is an unusual way to access the original/unpatched globalThis.performance. | ||
// This is needed to get hold of the real performance object before any of the unenv polyfills are | ||
// applied via `inject` or `polyfill` config in presets. | ||
// | ||
// This code relies on the that rollup/esbuild/webpack don't evaluate string concatenation | ||
// so they don't recognize the below as `globalThis.performance` which they would try to rewrite | ||
// into unenv/runtime/node/perf_hooks, thus creating a circular dependency, and breaking this polyfill. | ||
const workerdGlobalPerformance = (globalThis as any)[ | ||
"perf" + "ormance" | ||
] as typeof nodePerfHooks.performance; | ||
|
||
// reuse unenv's polyfill, but since preserve globalThis.performance identity | ||
// we use `.bind(unenvPerformance)` here to preserve the `this` for all delegated method calls | ||
export const performance = Object.assign(workerdGlobalPerformance, { | ||
// @ts-expect-error undocumented public API | ||
addEventListener: unenvPerformance.addEventListener.bind(unenvPerformance), | ||
clearMarks: unenvPerformance.clearMarks.bind(unenvPerformance), | ||
clearMeasures: unenvPerformance.clearMeasures.bind(unenvPerformance), | ||
clearResourceTimings: | ||
unenvPerformance.clearResourceTimings.bind(unenvPerformance), | ||
// @ts-expect-error undocumented public API | ||
dispatchEvent: unenvPerformance.dispatchEvent.bind(unenvPerformance), | ||
eventLoopUtilization: | ||
unenvPerformance.eventLoopUtilization.bind(unenvPerformance), | ||
getEntries: unenvPerformance.getEntries.bind(unenvPerformance), | ||
getEntriesByName: unenvPerformance.getEntriesByName.bind(unenvPerformance), | ||
getEntriesByType: unenvPerformance.getEntriesByType.bind(unenvPerformance), | ||
mark: unenvPerformance.mark.bind(unenvPerformance), | ||
markResourceTiming: | ||
// @ts-expect-error undocumented public API | ||
unenvPerformance.markResourceTiming.bind(unenvPerformance), | ||
measure: unenvPerformance.measure.bind(unenvPerformance), | ||
nodeTiming: { ...unenvPerformance.nodeTiming }, | ||
onresourcetimingbufferfull: | ||
// @ts-expect-error undocumented public API | ||
typeof unenvPerformance.onresourcetimingbufferfull === "function" | ||
? // @ts-expect-error undocumented public API | ||
unenvPerformance.onresourcetimingbufferfull.bind(unenvPerformance) | ||
: // @ts-expect-error undocumented public API | ||
unenvPerformance.onresourcetimingbufferfull, | ||
removeEventListener: | ||
// @ts-expect-error undocumented public API | ||
unenvPerformance.removeEventListener.bind(unenvPerformance), | ||
setResourceTimingBufferSize: | ||
unenvPerformance.setResourceTimingBufferSize.bind(unenvPerformance), | ||
timerify: unenvPerformance.timerify.bind(unenvPerformance), | ||
toJSON: unenvPerformance.toJSON.bind(unenvPerformance), | ||
}); | ||
|
||
export default { | ||
/** | ||
* manually unroll unenv-polyfilled-symbols to make it tree-shakeable | ||
*/ | ||
Performance, | ||
PerformanceEntry, | ||
PerformanceMark, | ||
PerformanceMeasure, | ||
// @ts-expect-error TODO: resolve type-mismatch between web and node PerformanceObserverEntryList | ||
PerformanceObserverEntryList, | ||
PerformanceObserver, | ||
// @ts-expect-error TODO: resolve type-mismatch between web and node PerformanceObserverEntryList | ||
PerformanceResourceTiming, | ||
constants, | ||
createHistogram, | ||
monitorEventLoopDelay, | ||
|
||
/** | ||
* manually unroll workerd-polyfilled-symbols to make it tree-shakeable | ||
*/ | ||
performance, | ||
} satisfies typeof nodePerfHooks; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.