-
Notifications
You must be signed in to change notification settings - Fork 5.6k
feat(unstable): --unstable-detect-cjs
for respecting explicit "type": "commonjs"
#26149
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(unstable): --unstable-detect-cjs
for respecting explicit "type": "commonjs"
#26149
Conversation
"type": "commonjs"
--unstable-cjs-detection
flag for respecting explicit "type": "commonjs"
--unstable-cjs-detection
flag for respecting explicit "type": "commonjs"
--unstable-cjs-detection
for respecting explicit "type": "commonjs"
cli/args/flags.rs
Outdated
} else if self.idx > 3 { | ||
let granular_flag = crate::UNSTABLE_GRANULAR_FLAGS.get(self.idx - 4)?; | ||
} else if self.idx > 4 { | ||
let granular_flag = crate::UNSTABLE_GRANULAR_FLAGS.get(self.idx - 5)?; |
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.
Would be better if the code didn't require changing numbers like this.
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.
CC @crowlKats
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 could just be an else
case instead, though the subtraction can't be avoided
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.
There is already an Why do we do this instead of just constructing a vector or pushing to a command?else
.
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 removed this code in fdbb7a4
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.
Gave this a go on a test project and it works great! The config schema in cli/schemas/config-file.v1.json
might also need to be update to get that fancy autocompletion for the unstable features.
--unstable-cjs-detection
for respecting explicit "type": "commonjs"
--unstable-detect-cjs
for respecting explicit "type": "commonjs"
cli/args/flags.rs
Outdated
Arg::new("unstable-detect-cjs") | ||
.long("unstable-detect-cjs") | ||
.help("Reads the package.json type field in a project to treat .js files as .cjs") | ||
.env("DENO_UNSTABLE_PACKAGE_JSON_TYPE") |
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.
Do we really need it? Is it used only for testing? If so, maybe we should handle it manually so it doesn't show up in --help
output?
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.
Oops, this should be the new name, but also I don't think we should bother with these env vars. It probably hurts startup time and I bet nobody uses it.
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 agree, let's remove it then.
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.
Overall looks good - could you please add some test cases for deno compile
? It appears there were changes in that code as well that are currently not exercised.
I'll add a message to say it's not supported in deno compile (right now it will crash in most cases). |
Sounds good. |
When using the
--unstable-detect-cjs
flag or adding"unstable": ["detect-cjs"]
to a deno.json, it will make a JS file CJS if the closest package.json contains"type": "commonjs"
and the file is not an ESM module (no TLA, noimport.meta
, noimport
/export
).--unstable-detect-cjs
tracking issue #26225