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(cli/tools/installer): allow remote import maps #10499

Merged
merged 2 commits into from May 10, 2021

Conversation

satyarohith
Copy link
Member

@satyarohith satyarohith commented May 5, 2021

The previous version fetched the source of the specified local import map and recreated a new import map alongside the bin script.
This commit removes the old behavior and directly passes the specified import map URL to deno run without creating a new one.
This will enable us to support remote import maps for deno install.

Closes #10482

@lucacasonato lucacasonato requested a review from kt3k May 5, 2021 12:54
@kt3k
Copy link
Member

kt3k commented May 6, 2021

I think this doesn't work for the local import map files. For example,

./target/debug/deno install -f --import-map=./import_map.json https://deno.land/std/examples/welcome.ts

This command creates the following installed shell script:

#!/bin/sh
# generated by deno install
exec deno run --import-map ./import_map.json 'https://deno.land/std/examples/welcome.ts' "$@"

So this installed script use different import map dependeng on the current directory.

I think we should change the behavior depending on the given import map path. And we also need test cases for this change.

@satyarohith satyarohith force-pushed the installer/fix_remote_import_map branch from dd78e82 to d0ef531 Compare May 7, 2021 05:09
@satyarohith
Copy link
Member Author

I made it similar to how scripts are installed. It will now have an absolute file URL for relative paths. And I also added a test.

@satyarohith satyarohith force-pushed the installer/fix_remote_import_map branch from d0ef531 to 659f9f1 Compare May 7, 2021 06:02
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

@satyarohith Thank you for updating and adding tests! LGTM

}

let content = fs::read_to_string(file_path).unwrap();
assert!(content.contains(&expected_string));
Copy link
Member

Choose a reason for hiding this comment

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

👍

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.

deno install with an import map from a URL
2 participants