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(unstable): ability to resolve specifiers with no extension, specifiers for a directory, and TS files from JS extensions #21464

Merged
merged 18 commits into from
Dec 7, 2023

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented Dec 5, 2023

This PR adds an --unstable-sloppy-imports flag which supports the following for file: specifiers:

  • Allows writing ./mod in a specifier to do extension probing.
    • ex. import { Example } from "./example" instead of import { Example } from "./example.ts"
  • Allows writing ./routes to do directory extension probing for files like ./routes/index.ts
  • Allows writing ./mod.js for mod.ts files.

This functionality is NOT RECOMMENDED for general use with Deno:

  1. It's not as optimal for perf: https://marvinh.dev/blog/speeding-up-javascript-ecosystem-part-2/
  2. It makes tooling in the ecosystem more complex in order to have to understand this.
  3. The "Deno way" is to be explicit about what you're doing. It's better in the long run.
  4. It doesn't work if published to the Deno registry because doing stuff like extension probing with remote specifiers would be incredibly slow.

This is instead only recommended to help with migrating existing projects to Deno. For example, it's very useful for getting CJS projects written with import/export declaration working in Deno without modifying module specifiers and for supporting TS ESM projects written with ./mod.js specifiers.

This feature will output warnings to guide the user towards correcting their specifiers. Additionally, Quick fixes are provided in the LSP to update these specifiers:

Shows a quick fix to update the specifier to the proper one

@@ -447,6 +447,7 @@ pub struct Flags {
pub unstable: bool,
pub unstable_bare_node_builtins: bool,
pub unstable_byonm: bool,
pub unstable_loose_imports: bool,
Copy link
Member Author

Choose a reason for hiding this comment

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

Alternative name to further discourage use: --unstable-sloppy-imports

Copy link
Member

@ry ry Dec 5, 2023

Choose a reason for hiding this comment

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

I like sloppy imports.

unstable-chaotic-imports?
unstable-messy-imports?
unstable-reckless-imports ?
unstable-naughty-imports?

Copy link
Member

Choose a reason for hiding this comment

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

I also like --unstable-sloppy-imports better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

@dsherret
Copy link
Member Author

dsherret commented Dec 5, 2023

This should be good to go now. I'll add the functionality for the informative error messages when not using this flag in a separate PR.

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.

FAN-TA-STIC

Comment on lines +1199 to +1218
fn specifier_text_for_redirected(
redirect: &lsp::Url,
referrer: &lsp::Url,
) -> String {
if redirect.scheme() == "file" && referrer.scheme() == "file" {
// use a relative specifier when it's going to a file url
match referrer.make_relative(redirect) {
Some(relative) => {
if relative.starts_with('.') {
relative
} else {
format!("./{}", relative)
}
}
None => redirect.to_string(),
}
} else {
redirect.to_string()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Very cool 👍

cli/lsp/documents.rs Outdated Show resolved Hide resolved
cli/resolver.rs Outdated Show resolved Hide resolved
@dsherret dsherret enabled auto-merge (squash) December 6, 2023 23:46
@dsherret dsherret merged commit 890780a into denoland:main Dec 7, 2023
14 checks passed
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.

3 participants