-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
feat: support crate imports in deno_typescript #3814
Conversation
b2f768c
to
c62cc26
Compare
5474367
to
c25f2ef
Compare
#[macro_export] | ||
macro_rules! crate_modules { | ||
() => { | ||
pub const DENO_CRATE_PATH: &'static str = env!("CARGO_MANIFEST_DIR"); | ||
}; | ||
} | ||
|
||
#[macro_export] | ||
macro_rules! include_crate_modules { | ||
( $( $x:ident ),* ) => { | ||
{ | ||
let mut temp: HashMap<String, String> = HashMap::new(); | ||
$( | ||
temp.insert(stringify!($x).to_string(), $x::DENO_CRATE_PATH.to_string()); | ||
)* | ||
temp | ||
} | ||
}; | ||
} |
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.
This solution is so simple that these macros almost feel like extraneous sugar 👍
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.
@ry I think once https://github.com/denoland/deno/pull/3814/files#r373159795 is resolved we could land it and try to factor out dispatch_json
using this mechanism
c25f2ef
to
5455b51
Compare
@bartlomieju I'm going to refactor #3692 to use this once it lands. |
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.
It would be very cool if these files could be accessed without using include_str.
Although this patch is small I'm hesitant to introduce a new URL schema like this... I'm marking this as "request changes" because I'm not ready to land it yet. I'm not sure if this is actually possible without include _str ...
Sorry @afinch7 I guess the PR doesn't use Anyway the solution looks good now that I've seen it clearly. I wonder if this works with cargo package correctly? |
Practical example: import { makeTempDir } from "crate:https://deno_fs/make_temp_dir.ts"; |
We should really add an explanation to the manual. |
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.
Yes, very elegant! +1
Just need some a minimal manual entry and then we can land it |
@ry this is snapshot only feature, does it really need entry in manual right now? Maybe let's refactor some crates and stabilize this feature first? |
Alright - you're right - let's land it. |
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!
Basic support for crate imports for
deno_typescript
. Syntax is like thiscrate:https://crate_name/path_in_crate.ts
.