-
Notifications
You must be signed in to change notification settings - Fork 44
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: load bytes instead of strings #357
Conversation
Esm(EsModule), | ||
// todo(#239): remove this when updating the --json output for 2.0 | ||
#[serde(rename = "esm")] | ||
Js(JsModule), |
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.
I found it simplifies the downstream code to use Module::Js
here and then we can introduce Module::Wasm
later. The benefit is that it forces people to handle Wasm modules specifically and also if someone has a JsModule
then they don't need to worry about if they can get a string out of it or not.
Couldn't think of a better name because ScriptModule
has the connotations that it's not an ES module. Another alternative is JsTsModule
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.
Current name is fine 👍
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.
Looks like the JS implementation got out of sync. Other than that it looks good to me 👍
Should be good now. I'm going to finish upgrading this PR into the downstream crates, then do a release and upgrade everything. Then I'll circle back and add Wasm imports. |
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
src/graph.rs
Outdated
#[allow(clippy::result_large_err)] | ||
fn new_source_with_text( | ||
specifier: &ModuleSpecifier, | ||
text: Arc<[u8]>, | ||
) -> Result<Arc<str>, ModuleError> { |
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.
Unrelated to this PR, but maybe we could box the error to get rid of the lint?
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.
I'll box at least this code for now, but yeah we should box more of these errors.
This is in preparation for
with { "type": "wasm" }
and having the ability to get the original bytes.Closes #152