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: use primordials for extensions/timers #11234

Closed
wants to merge 3 commits into from
Closed

refactor: use primordials for extensions/timers #11234

wants to merge 3 commits into from

Conversation

SimonRask
Copy link
Contributor

Towards #11224

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove OriginalDateNow from line 303, and replace that with DateNow from primordials?

There is also a pendingFireTimers.shift() on line 331, a pendingFireTimers.indexOf on line 415, and a pendingFireTimers.splice on line 416. Also line 443 and 445, amd a idMap.has on line 452, a idMap.delete on line 458, an eval on line 472, a TypeError on 478, a Function.prototype.bind.call on line 493, an idMap.set on 523, and an idMap.get on 551, idMap.delete on 558.

@lucacasonato lucacasonato mentioned this pull request Jul 2, 2021
67 tasks
@SimonRask
Copy link
Contributor Author

Alright sorry for the hassle @lucacasonato

Having some trouble decoding how line 493 should be rewritten:
Function.prototype.bind.call(cb, globalThis, ...args)

Thought it would be something like this but it doesn't seem like it:
FunctionPrototypeCall(FunctionPrototypeBind, cb, globalThis, ...args)

Also I don't know if I'm looking in the wrong place, but I don't see eval in internal.d.ts

@@ -480,7 +498,7 @@
let callback;

if ("function" === typeof cb) {
callback = Function.prototype.bind.call(cb, globalThis, ...args);
callback = FunctionPrototypeBind(cb, globalThis, ...args);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucacasonato
I'm not too familiar or sure about this, but the tests seem to pass

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