-
Notifications
You must be signed in to change notification settings - Fork 27
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: eszip v2 #40
feat: eszip v2 #40
Conversation
b3f9704
to
e1418e1
Compare
Co-authored-by: Divy Srivastava <[email protected]>
e1418e1
to
1471097
Compare
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! A few nitpicks, but it looks great overall
} | ||
let start = read; | ||
read += $n; | ||
&header_bytes[start..read] |
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.
Since this is a macro, it could created a fixed array and copy &header_bytes[start..read]
into it. This will eliminate the .try_into().unwrap()
calls below
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.
Can't do, because read!
is invoked with a non const length, for example read!(specifier_len, "specifier")
. The length of an array needs to be const.
source_map: EszipV2SourceSlot, | ||
}, | ||
Redirect { | ||
target: String, |
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.
Consider using Cow<'a, str>
instead?
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 would require that the EszipV2Module
structs have a lifetime. I don't think that can be achieved easily.
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!
This PR is a first pass a new iteration of the ESZip format. This PR also changes the
purpose of this crate: it is now exclusively responsible for three things:
Eszip
struct that can contain either a V1, or V2 eszip, but exposes a unified module loading API.deno_graph::ModuleGraph
into a V2 eszipThe crate is not anymore responsible for module loading itself, and does not anymore contain a module loader.