refactor: remove PermissionsContainer in deno_runtime
#24119
Merged
Conversation
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
dsherret
commented
Jun 6, 2024
| let code_source = if let Some(result) = self | ||
| .shared | ||
| .npm_module_loader | ||
| .load_if_in_npm_package(specifier, maybe_referrer, permissions) | ||
| .load_if_in_npm_package(specifier, maybe_referrer) |
There was a problem hiding this comment.
The permissions were only used for cjs resolution for loading a package.json, which was kind of useless.
dsherret
commented
Jun 6, 2024
| path: &Path, | ||
| api_name: Option<&str>, | ||
| ) -> Result<(), AnyError>; | ||
| } | ||
|
|
||
| pub(crate) struct AllowAllNodePermissions; | ||
| pub struct AllowAllNodePermissions; |
There was a problem hiding this comment.
I exposed this because it's faster to use this trait than construct a new PermissionsContainer.
dsherret
commented
Jun 6, 2024
| state.put(Rc::new(NodeResolver::new( | ||
| fs, | ||
| npm_resolver, | ||
| ))) |
There was a problem hiding this comment.
This has an internal in_npm_package_cache that we can share across workers by passing the single instance into here.
bartlomieju
reviewed
Jun 6, 2024
| referrer, | ||
| DEFAULT_CONDITIONS, | ||
| mode, | ||
| permissions, |
There was a problem hiding this comment.
Are you sure these were only used for package.json? I thought we used them when probing for various file extensions
There was a problem hiding this comment.
It hasn't been from what I can tell, but even if it were I don't think we should bother permission checking that?
littledivy
approved these changes
Jun 7, 2024
nathanwhit
pushed a commit
that referenced
this pull request
Jun 13, 2024
Also removes permissions being passed in for node resolution. It was completely useless because we only checked it for reading package.json files, but Deno reading package.json files for resolution is perfectly fine. My guess is this is also a perf improvement because Deno is doing less work.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Also removes permissions being passed in for node resolution. It was completely useless because we only checked it for reading package.json files, but Deno reading package.json files for resolution is perfectly fine.
My guess is this is also a perf improvement because Deno is doing less work.