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

feat(cli): Replace bundling with eszip in deno compile #13563

Conversation

williamtetlow
Copy link
Contributor

@williamtetlow williamtetlow commented Feb 1, 2022

#13430

Replace bundling with eszip in deno compile.

Also adds support for dynamic imports in standalone executables #8655

@CLAassistant
Copy link

CLAassistant commented Feb 1, 2022

CLA assistant check
All committers have signed the CLA.

@williamtetlow williamtetlow marked this pull request as draft February 1, 2022 21:00
@williamtetlow williamtetlow changed the title feat(cli): Replace bundling with eszip in deno compile [WIP] feat(cli): Replace bundling with eszip in deno compile Feb 1, 2022
@bartlomieju
Copy link
Member

@littledivy please take a look

@williamtetlow
Copy link
Contributor Author

williamtetlow commented Feb 2, 2022

@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

@williamtetlow williamtetlow force-pushed the williamtetlow/replace-bundling-with-eszip-for-deno-compile branch from 891f57f to 59b2fad Compare February 3, 2022 17:18
@williamtetlow williamtetlow changed the title [WIP] feat(cli): Replace bundling with eszip in deno compile feat(cli): Replace bundling with eszip in deno compile Feb 3, 2022
@williamtetlow
Copy link
Contributor Author

@littledivy @bartlomieju this is ready for review

@williamtetlow williamtetlow marked this pull request as ready for review February 3, 2022 17:22
@williamtetlow williamtetlow force-pushed the williamtetlow/replace-bundling-with-eszip-for-deno-compile branch from 5a85740 to 4f59634 Compare February 3, 2022 18:12
Copy link
Member

@bartlomieju bartlomieju left a 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!");

Cargo.lock Outdated Show resolved Hide resolved
err.print().unwrap();
std::process::exit(0);

let exit_code = async move {
Copy link
Member

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

Copy link
Contributor Author

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.

cli/standalone.rs Show resolved Hide resolved
Copy link
Member

@littledivy littledivy left a 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:///Users/divy/gh/deno/a.ts
Compile file:///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.

cli/standalone.rs Outdated Show resolved Hide resolved
@williamtetlow
Copy link
Contributor Author

I think this change will allow us to support dynamic imports in standalone executables too f2b55e8

@bartlomieju bartlomieju mentioned this pull request Feb 5, 2022
@littledivy littledivy self-assigned this Feb 14, 2022
Cargo.lock Outdated Show resolved Hide resolved
Copy link
Member

@littledivy littledivy left a 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

Copy link
Member

@bartlomieju bartlomieju left a 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

@vectronic
Copy link

@littledivy @williamtetlow

I'm curious as to what changed in this PR that meant you had to remove this test:

0ff2ce0

I assume you originally had this working due to this comment:

#13563 (comment)

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.

None yet

5 participants