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(swc/cli): initial compile command implementation #3602

Merged
merged 2 commits into from
Feb 17, 2022

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Feb 16, 2022

Description:

We have plugin subcommand, now we can work on other subcommand. This is initial effort to port over @swc/cli's compilation command features under compile subcommand. It doesn't cover all of existing cli args yet, only covers most basic execution paths.

There are a couple of challenges we need to deal in the future. First, output path calculation relates to the input. This is not unique to this cli, as we have known issues like #3028 already. However, it's something we need to take care of.

Second one is more tricky - plugin options for the js-written plugins. Previously we can rely on node.js runtime as cli runs on those, which new cli doesn't. I don't have great idea yet except either deprecate js plugin or spin node.js separately to get transformed output from those. Both are not great.

Related issue (if exists):


let dist_absolute_path = out_dir.absolutize()?;

// These are possible combinations between input to output dir.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is note for the future what we'll need to support for the path calculation.


files
.into_par_iter()
.try_for_each_with(cm, |compiler, file_path| {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I choose naive parallel iterator for processing files, maybe there's better / recommended practices?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parallel iterator is the most ideal one if we have a list of files to process.

#[clap(name = "SWC", version)]
#[clap(global_setting(AppSettings::PropagateVersion))]
#[clap(global_setting(AppSettings::UseLongFormatForHelpSubcommand))]
#[clap(name = "SWC", version, propagate_version = true)]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://epage.github.io/blog/2022/02/clap-31-a-step-towards-40/ and happy to see clap moving towards a modular approach.

Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

serde = { version = "1", features = ["derive"] }
serde_json = { version = "1", features = ["unbounded_depth"] }

[dependencies.path-absolutize]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I love this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is not a complete solution but reasonably works well to cover some cases as far as I could check. Still, as written in path resolution logic I believe we'll need further improvement though.

.filter(|e| {
extensions.iter().any(|ext| {
e.extension()
.map(|v| v.to_str().unwrap_or("").eq(ext))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OsStr implements PartialEq<str>. (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#impl-PartialEq%3Cstr%3E)

You can do v == &**ext instead.

let base = file_path
.parent()
.unwrap_or(&default)
.as_os_str()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think .display().to_string() is better.


files
.into_par_iter()
.try_for_each_with(cm, |compiler, file_path| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parallel iterator is the most ideal one if we have a list of files to process.

Copy link
Member

@kdy1 kdy1 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!


swc-bump:

  • swc_cli

@kdy1 kdy1 enabled auto-merge (squash) February 17, 2022 09:59
@kdy1 kdy1 merged commit 5ed3f3a into swc-project:main Feb 17, 2022
@kwonoj kwonoj deleted the feat-cli-compile-files branch February 22, 2022 17:56
@swc-project swc-project locked as resolved and limited conversation to collaborators Oct 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants