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

Add redirect to the fetch #2561

Merged
merged 6 commits into from Jun 24, 2019

Conversation

3 participants
@TonyLianLong
Copy link
Contributor

commented Jun 22, 2019

This closes #2513. It's my first time submitting a PR to the deno project, and if there is anything to change, feel free to tell me.

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

@TonyLianLong thanks for contribution! I see 2 potential problems with your solution:

  1. redirect loop - there's no fail safe for too many redirects
  2. it's implemented in TS issuing multiple fetch ops, we definitely should explore possibility to implement in in Rust
@ry
Copy link
Collaborator

left a comment

Thanks! Nice work!

Just a few comments..

Show resolved Hide resolved js/fetch.ts Outdated
Show resolved Hide resolved js/fetch.ts Outdated
Show resolved Hide resolved js/fetch_test.ts
@TonyLianLong

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

@bartlomieju

  1. it's implemented in TS issuing multiple fetch ops, we definitely should explore possibility to implement in in Rust

I agree that it's more efficient when we implement this in Rust and we should definitely consider the possibility of handling all the redirection process in Rust. But I believe it is simpler to write the logic in TypeScript just for now (as you can see, it's about 10 lines for the whole implementation). In addition, I wonder whether writing the redirection in Rust may make it more prone to security issues (e.g. redirection to some CORS which bypasses the check) in the future when we have more and more contributions and modifications. If the situation is possible, we can prevent this type of bugs by making a new request from TypeScript side for every redirection that we need to do.

@ry

ry approved these changes Jun 24, 2019

Copy link
Collaborator

left a comment

LGTM - thanks!

Generally, If possible, I prefer that things are implemented in TS rather than Rust. The API for binding Rust into TS is not yet ideal and I expect that it will be changed soon. This is why the HTTP server is implemented in TS, when we could get, probably, a large perf boost by binding to hyper instead. Once that interface is stable, we can invest in perf improvements, but starting with areas where we can measure the problem (like in the HTTP server).

(In particular, I want the native op interface cli/ops.rs to be the same as that for dlopen'd code and WASM code...)

@ry ry merged commit 1d0d542 into denoland:master Jun 24, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@TonyLianLong TonyLianLong deleted the TonyLianLong:add-redirect branch Jun 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.