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

fix(ext/timers): create primordial eval #15110

Merged
merged 11 commits into from Sep 2, 2022
Merged

fix(ext/timers): create primordial eval #15110

merged 11 commits into from Sep 2, 2022

Conversation

vimirage
Copy link
Contributor

@vimirage vimirage commented Jul 6, 2022

Includes eval in primordials; this was needed to prevent user modification of timers execution.
Alternatively it could've used Deno.core.evalContext?

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

could you add a test?

@vimirage

This comment was marked as outdated.

@vimirage
Copy link
Contributor Author

@ry May you review this again?

Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

LGTM: I might go with some other name like primordialEval but that's just a nit.

@bartlomieju
Copy link
Member

@phosra any chance you could update prefer-primordials rule in deno_lint, so we are guarded against accidental mistakes of using eval from a global scope?

@vimirage
Copy link
Contributor Author

vimirage commented Jul 31, 2022

@phosra any chance you could update prefer-primordials rule in deno_lint, so we are guarded against accidental mistakes of using eval from a global scope?

I opened an issue about it there about a week ago: denoland/deno_lint#1062.
I wasn't certain how rules such as Set -> primordials.SafeSet were handled, so I held off, but sure - I can make an attempt anyways.

(Nonetheless, must that hold off this bug fix?)

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @phosra, nice fix

@vimirage
Copy link
Contributor Author

vimirage commented Sep 2, 2022

Checks pass, this should be clear to merge.

@bartlomieju bartlomieju merged commit 4ec213b into denoland:main Sep 2, 2022
@vimirage vimirage deleted the patch-1 branch September 3, 2022 05:53
kt3k pushed a commit that referenced this pull request Sep 9, 2022
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.

None yet

5 participants