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): --ext parameter for run, compile, and bundle #17172

Merged
merged 37 commits into from
Mar 22, 2023

Conversation

Cre3per
Copy link
Contributor

@Cre3per Cre3per commented Dec 23, 2022

Adds --ext to deno run, closes #5088

Additionally

  • Adds --ext to deno compile and deno bundle
  • Deprecates deno eval --ts (Was already marked for removal in code, this PR exposes the deprecation to the CLI)
  • ⚠️ Changes the default value of deno fmt --ext from ts to js for consistency. If CLI default arguments are part of deno's API, this violates semver and needs to be undone. Personally, I'd prefer TypeScript to be the default for all commands, but JavaScript was chosen here Request: CLI flag to set language mode #5088 (comment). Changing the default value to TypeScript would not avoid the breaking change, because the main branch is inconsistent already (fmt vs eval).

You'll find a new TODO concerning deno_ast in the changes. I intend to address that TODO before this PR is merged, after I get confirmation that the PR is otherwise ok.

dsherret added a commit that referenced this pull request Jan 4, 2023
@kt3k
Copy link
Member

kt3k commented Jan 5, 2023

Is it correct for #17269 to close this? @dsherret

@dsherret
Copy link
Member

dsherret commented Jan 5, 2023

Only if it increases my closed issue count.

@dsherret dsherret reopened this Jan 5, 2023
@dsherret
Copy link
Member

dsherret commented Jan 5, 2023

Thanks and sorry, this was done in error. I meant to close #17112, when I copy and pasted the issue number gh did some auto complete weirdness that must have selected this one.

bartlomieju pushed a commit that referenced this pull request Jan 5, 2023
@bartlomieju bartlomieju added this to the 1.31 milestone Jan 28, 2023
Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

Please resolve conflicts.

For what it's worth, the changes themselves seem fine to me. Consistency is good, and the breaking change here is very minor since it only affects pipe operation and extensionless input files. As such, I'd be willing to "look the other way" in terms of breaking semver. I do not decide that though.

cli/args/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

Looks a lot better! Thanks! I have just a few comments.

cli/args/flags.rs Outdated Show resolved Hide resolved
cli/args/flags.rs Outdated Show resolved Hide resolved
cli/args/flags.rs Outdated Show resolved Hide resolved
cli/args/flags.rs Outdated Show resolved Hide resolved
cli/args/flags.rs Outdated Show resolved Hide resolved
cli/args/flags.rs Outdated Show resolved Hide resolved
cli/args/mod.rs Outdated Show resolved Hide resolved
cli/args/mod.rs Outdated Show resolved Hide resolved
cli/cache/mod.rs Outdated Show resolved Hide resolved
@dsherret dsherret added this to the 1.32 milestone Mar 9, 2023
@Cre3per Cre3per requested a review from dsherret March 15, 2023 12:25
Copy link
Member

@dsherret dsherret 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 @Cre3per!

@Cre3per
Copy link
Contributor Author

Cre3per commented Mar 22, 2023

I had a look at your commits, looks good. Thanks for your feedback @dsherret!

@dsherret
Copy link
Member

@Cre3per and sorry for my delay on this one! I did a few commits just now because I want to get this in the release (happening in a little bit).

@dsherret dsherret merged commit fd0658f into denoland:main Mar 22, 2023
@kt3k
Copy link
Member

kt3k commented Mar 23, 2023

Now deno run - seems handling the stdin as JavaScript by default:

$ echo "console.log(1 as any)" | deno run  -        
error: The module's source code could not be parsed: Expected ',', got 'as' at file:https:///Users/kt3k/denoland/deno_std/$deno$stdin:1:15

  console.log(1 as any)
                ~~
$ echo "console.log(1 as any)" | deno run --ext ts -
1
$ deno -V
deno 1.32.0

Is this expected? (Some test cases in std failed by this change)

@Cre3per
Copy link
Contributor Author

Cre3per commented Mar 23, 2023

@kt3k That's unintentional. I'm adding a test case for deno run - and working on a fix.

UPDATE: Fixed in #18391

dsherret pushed a commit that referenced this pull request Mar 23, 2023
Bug reported here shortly after merging `--ext` changes
#17172 (comment)

Also found a missing `--check` in integration tests for `--ext` that
would have missed a bug if there was one.

Fixes #18392
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.

Request: CLI flag to set language mode
6 participants