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/lock): autodiscovery of lockfile #16498

Merged
merged 26 commits into from
Nov 2, 2022

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Oct 31, 2022

This commit adds autodiscovery of lockfile.

This only happens if Deno discovers the configuration file (either
"deno.json" or "deno.jsonc"). In such case Deno tries to load "deno.lock"
file that sits next to the configuration file, or creates one for user if
the lockfile doesn't exist yet.

As a consequence, "--lock" and "--lock-write" flags had been updated.
"--lock" no longer requires a value, if one is not provided, it defaults
to "./deno.lock" resolved from the current working directory. "--lock-write"
description was updated to say that it forces to overwrite a lockfile.

Autodiscovery is currently not handled by the LSP.

@@ -458,7 +458,9 @@ impl Flags {
Some(files.clone())
} else if let Run(RunFlags { script }) = &self.subcommand {
if let Ok(module_specifier) = deno_core::resolve_url_or_path(script) {
if module_specifier.scheme() == "file" {
if module_specifier.scheme() == "file"
|| module_specifier.scheme() == "npm"
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 was fun - I tried this PR with Vite and couldn't get it to write to a lockfile. Turns out we never discover configuration file is the entrypoint is a npm package (eg. deno run -A --unstable npm:vite). This conditional now allows to discover configuration file in such case. Should I factor it out to a separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, interesting. I think this makes sense to do.

@bartlomieju bartlomieju changed the title [WIP] feat: autodiscovery of lockfile feat: autodiscovery of lockfile Nov 1, 2022
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Should we remove --lock-write flag?

What about a default value for --lock?

If I change the lock file and then run again with --lock=deno.json I get this error:

# deno run --lock=deno.lock  --unstable -A  main.ts

============================================================
Deno has panicked. This is a bug in Deno. Please report this
at https://github.com/denoland/deno/issues/new.
If you can reliably reproduce this panic, include the
reproduction steps and re-run with the RUST_BACKTRACE=1 env
var set and include the backtrace in your report.

Platform: macos aarch64
Version: 1.27.0
Args: ["deno", "run", "--lock=deno.lock", "--unstable", "-A", "main.ts"]

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', cli/npm/resolution.rs:311:43
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@bartlomieju
Copy link
Member Author

bartlomieju commented Nov 1, 2022

Should we remove --lock-write flag?

No, it allows to overwrite the lockfile completely - eg. when you get into a situation where there's integrity mismatch, but want to update the lockfile, then specifying this flag will do the job. Error message suggests it:

error: Integrity check failed for npm package: "cowsay@1.5.0".
  Cache has "sha512-8Ipzr54Z8zROr/62C8f0PdhQcDusS05gKTS87xxdji8VbWefWly0k8BwGK7+VqamOrkv3eGsCkPtvlHzrhWsCA==" and lockfile has "foobar".
  Use "--lock-write" flag to update the lockfile.

What about a default value for --lock?

What would be the default value? CWD/deno.lock?

If I change the lock file and then run again with --lock=deno.json I get this error:

Can you provide contents of deno.lock and main.ts?

@ry
Copy link
Member

ry commented Nov 1, 2022

What would be the default value? CWD/deno.lock?

yes

Can you provide contents of deno.lock and main.ts?

https://gist.github.com/ry/f313e2d8a56139e8ecb5fa7994d885e4

@bartlomieju
Copy link
Member Author

@ry all these problems have now been addressed.

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! Will be exciting to have this.

cli/args/flags.rs Outdated Show resolved Hide resolved
Some(config_file) => {
if config_file.specifier.scheme() == "file" {
let mut path = config_file.specifier.to_file_path().unwrap();
path.set_file_name("deno.lock");
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering if this should be deno-lock.json like package-lock.json, but I think this makes sense because then the formatter doesn't pick it up.

cli/lockfile.rs Outdated Show resolved Hide resolved
cli/lsp/language_server.rs Outdated Show resolved Hide resolved
@bartlomieju bartlomieju changed the title feat: autodiscovery of lockfile fix(lock): autodiscovery of lockfile Nov 2, 2022
@bartlomieju bartlomieju merged commit 5dea510 into denoland:main Nov 2, 2022
@bartlomieju bartlomieju deleted the auto_discover_lockfile branch November 2, 2022 15:32
@0f-0b
Copy link
Contributor

0f-0b commented Nov 3, 2022

Is there a way to disable this behavior similar to how --no-config disables auto-discovery of config files?

@bartlomieju
Copy link
Member Author

Is there a way to disable this behavior similar to how --no-config disables auto-discovery of config files?

Not currently, no. Is there a reason why you don't want to use a lockfile?

@dsherret dsherret changed the title fix(lock): autodiscovery of lockfile feat(unstable/lock): autodiscovery of lockfile Nov 3, 2022
DjDeveloperr pushed a commit to DjDeveloperr/deno that referenced this pull request Nov 4, 2022
This commit adds autodiscovery of lockfile. 

This only happens if Deno discovers the configuration file (either 
"deno.json" or "deno.jsonc"). In such case Deno tries to load
"deno.lock"
file that sits next to the configuration file, or creates one for user
if
the lockfile doesn't exist yet.

As a consequence, "--lock" and "--lock-write" flags had been updated.
"--lock" no longer requires a value, if one is not provided, it defaults
to "./deno.lock" resolved from the current working directory.
"--lock-write"
description was updated to say that it forces to overwrite a lockfile.

Autodiscovery is currently not handled by the LSP.
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

4 participants