-
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(cli): Replace bundling with eszip in deno compile #13563
feat(cli): Replace bundling with eszip in deno compile #13563
Conversation
@littledivy please take a look |
@littledivy @bartlomieju this isn't ready for review still very much WIP, haven't started impl. yet. I was making draft PR but it auto added you as reviewer. I will tag you once it's ready for review |
891f57f
to
59b2fad
Compare
@littledivy @bartlomieju this is ready for review |
5a85740
to
4f59634
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.
@williamtetlow looks good! Before landing we should ensure that stack traces behave correctly. Can you add an integration tests that compiles following script?
// a.ts
import "./b.ts";
// b.ts
console.log("hello");
throw new Error("boom!");
err.print().unwrap(); | ||
std::process::exit(0); | ||
|
||
let exit_code = async move { |
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 seems like unrelated change. I suggest to revert
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.
@bartlomieju this was done as both extract_standalone
and standalone::run
have been converted to async functions.
I wrapped the calls to these and also the call to the async fn get_subcommand
in an async block so we call run_basic
only once.
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.
JSON import assertions no longer work.
~/g/deno (williamtetlow/replace-bundling-with-eszip-for-deno-compile)> target/release/deno compile -A ./a.ts
Check file:https:///Users/divy/gh/deno/a.ts
Compile file:https:///Users/divy/gh/deno/a.ts
Emit a
~/g/deno (williamtetlow/replace-bundling-with-eszip-for-deno-compile)> ./a
error: Expected a "JSON" module but loaded a "JavaScript" module.
Co-authored-by: Divy Srivastava <[email protected]>
I think this change will allow us to support dynamic imports in standalone executables too f2b55e8 |
…place-bundling-with-eszip-for-deno-compile
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! Thanks @williamtetlow
@bartlomieju PTAL
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'm still not happy about changes to cli/main.rs
but I can live with them and refactor them later.
LGTM, nice work @williamtetlow
I'm curious as to what changed in this PR that meant you had to remove this test: I assume you originally had this working due to this comment: |
#13430
Replace bundling with
eszip
indeno compile
.Also adds support for dynamic imports in standalone executables #8655