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

Local filenames starting with 'http' shouldn't be remote. #1167

Merged
merged 2 commits into from
Nov 8, 2018

Conversation

ry
Copy link
Member

@ry ry commented Nov 7, 2018

"http_test.ts" was being marked as a remote module specifier.

"http_test.ts" was being marked as a remote module specifier.
@ry ry requested a review from piscisaureus November 7, 2018 15:39
Copy link
Contributor

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

LGTM but for one thought.

const ASSET_PREFIX: &str = "/$asset$/";

fn is_remote(module_name: &str) -> bool {
module_name.starts_with("http")
module_name.starts_with("http:https://") || module_name.starts_with("https://")
Copy link
Contributor

Choose a reason for hiding this comment

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

Though we don't support it ATM, would any URI with a protocol be a better test here, a RegEx of /^\S+:\/{2}/? That would trap file:https:// as well, but we don't handle that yet anyways.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya let’s wait until we support other protocols. I think this function goes away completely in #1039 anyway.

Copy link
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry merged commit 98e6366 into denoland:master Nov 8, 2018
ry added a commit to ry/deno that referenced this pull request Nov 12, 2018
- Update to TypeScript 3.1.6 (denoland#1177)
- Fixes Headers type not available. (denoland#1175)
- Reader/Writer to use Uint8Array not ArrayBufferView (denoland#1171)
- Fixes importing modules starting with 'http'. (denoland#1167)
- build: Use target/ instead of out/ (denoland#1153)
- Support repl multiline input (denoland#1165)
@ry ry mentioned this pull request Nov 12, 2018
ry added a commit that referenced this pull request Nov 12, 2018
- Update to TypeScript 3.1.6 (#1177)
- Fixes Headers type not available. (#1175)
- Reader/Writer to use Uint8Array not ArrayBufferView (#1171)
- Fixes importing modules starting with 'http'. (#1167)
- build: Use target/ instead of out/ (#1153)
- Support repl multiline input (#1165)
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

3 participants