-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Reorganize $DENO_DIR/deps folder and split by protocol #971
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
Conversation
src/deno_dir.rs
Outdated
get_cache_filename(self.deps_https.as_path(), j).as_ref(), | ||
) | ||
} | ||
// TODO(kevinkassimo): change this to support other protocols than http |
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.
perhaps should do:
"http" => ...
_ => unimplemented!();
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.
Good idea.
@ry Is this change of design appropriate? I might want to settle this PR sooner, or close it, or explicitly let someone else take care of it (having some urgent academic business need to deal with, and might have to pause my contribution to deno for about 1 month) |
@kevinkassimo Sorry for the slow response - I'm hesitating on this because it's not solving an actual bug. The point of this is that http and https could be serving different files - and therefore we should cache them separately? While this is true - it seems like a very bad idea for a website to have this behavior - and I'm not sure we should work around that... |
@ry I feel http and https might be a special case here that could share the same cache (e.g. I could make Otherwise, we might have to use other sorts of metadata to store the original protocol used for script retrieval. |
HTTP and HTTPS could easily serve different files if there's a man-in-the-middle attack. Imagine you're on some crappy free wifi that injects some advertising code into any javascript files downloaded over http, you run one insignificant sandboxed software project using deno that imports Also, if a server only supports one protocol or the other, it would be really easy to fool yourself: imagine you write a script that imports |
There is simplicity in an injective/invertible cache map, being able to know precisely where a cached file was from. The cost of downloading the same file referenced with http and https twice seems worth it. I had thought the bug this was solving was relative imports inside ts, which requires building the url from the cache location..? It would be good if this code could be generic over all schemes. |
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.
LGTM
I was dragging my feet on this, because I don't like the added complexity to the ~/.deno
tree, but I acknowledge the correctness. Thanks Kevin.
- Add URLSearchParams (denoland#1049) - Implement clone for FetchResponse (denoland#1054) - Use content-type headers when importing from URLs. (denoland#1020) - Use checkJs option, JavaScript will be type checked and users can supply JSDoc type annotations that will be enforced by Deno (denoland#1068) - Add separate http/https cache dirs to DENO_DIR (denoland#971) - Support https in fetch. (denoland#1100) - Add chmod/chmodSync on unix (denoland#1088) - Remove broken features: --deps and trace() (denoland#1103) - Ergonomics: Prompt TTY for permission escalation (denoland#1081)
- Add URLSearchParams (#1049) - Implement clone for FetchResponse (#1054) - Use content-type headers when importing from URLs. (#1020) - Use checkJs option, JavaScript will be type checked and users can supply JSDoc type annotations that will be enforced by Deno (#1068) - Add separate http/https cache dirs to DENO_DIR (#971) - Support https in fetch. (#1100) - Add chmod/chmodSync on unix (#1088) - Remove broken features: --deps and trace() (#1103) - Ergonomics: Prompt TTY for permission escalation (#1081)
Supposed to close #930
Reorganize
$DENO_DIR/deps
folder by creating protocol specific subdirectories.e.g.
should create folder
$DENO_DIR/deps/http/a.com
and$DENO_DIR/deps/https/b.com
I feel this might be also a necessary step for Deno to probably support other protocols in the future.
There is currently no test on TS side. To add tests, I hope I could get sample code from #930 (comment) to be hosted somewhere stable.