-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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: lockfiles #3231
feat: lockfiles #3231
Conversation
Use --lock-write=lock.json or --lock-check=lock.json on the command line.
💯! I wonder if the API should be --lock-write as a bool (and require it be used in conjunction with the other flag specifying the lock file)? When would you check and write with two distinct files? i.e.
|
@hayd Maybe... What is the behavior if deno is provided only |
@ry error! (with a message) |
@hayd, I implemented your flag suggestion. Should --lock-write be combined with --reload? Should |
I'm not sure, I think maybe not... we can decide later. Reason maybe not: someone else updates the lockfile in git, you update, you have an old url cached e.g. https://deno.land/std/foo.ts so you need to --reload BUT you want to ensure the lockfile is unmodified (that you are running the same configuration as your colleague)... if not you want it to error.
I'm not sure. Need to think about that. I suspect you should trust it as otherwise the lock file will be incredibly noisy/a pain, and it will cause issues in git etc. |
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.
Awesome, I'm wondering about ergonomics though.
It'd be nice if you could just specify --lock
file without explicit path (otherwise everyone will have different lock filename, might be a good idea to give people a cue towards common name).
$ deno --lock --lock-write ./my_script.ts
$ cat Deno.lock
{
"https://127.0.0.1:4545/cli/tests/subdir/print_hello.ts": "5c93c66125878389f47f4abcac003f4be1276c5223612c26302460d71841e287",
"https://127.0.0.1:4545/cli/tests/003_relative_import.ts": "bad"
}
That way --lock-write
wouldn't require --lock
flag.
Anyway LGTM, just some food for thought.
A lock file will correspond to a module, right? Building on that, it would be nice if the lock file was written next to the specified main module as such: |
I think most of the time when developing one does not care about the lock file. It's only during deployment and testing that this is important - and usually in that case the invocation is scripted. I think this explicit file name specification might not be such a problem. Also I don't like the idea of blessing a special file name like "Deno.lock". There are not any special filenames in Deno, I want to strive to not introduce any if possible. Then again - I could be wrong - I think we just have to play with this a bit and see what the ergonomics are like in real-life. |
I think this should work with fetch as well. |
@hayd Do you mean |
I think another option is deps.lock.json so you'd do
That way forcing all external imports be included in deps.ts. I don't think we need to pick a winner, I also suspect higher level tooling will control this, along with import maps for a nicer experience. There is an interesting question re lockfiles as trees, i.e. if I import x/foo what files did it expect as its lockfile. Again, I think you could do lockfile "verification" as higher level, esp. since you can already get the dep tree separately. Edit: Perhaps this mentioned in a different thread (for some reason I was thinking about it), the idea was if you import from a registry e.g. |
was just checking. I think this is looking great. |
.takes_value(true) | ||
.global(true), | ||
).arg( | ||
Arg::with_name("lock-write") |
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 think it may be more user friendly to do deno --lock-write lockfile.json
rather than deno --lock lockfile.json --lock-write
.
I'd be okay with doing this in in a follow-up PR tho.
Sorry for the delay; I needed some time to consider whether this implementation would interfere with a potential future addition of signature support, but I've come to the conclusion that there isn't a problem. |
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.
LGTM with one comment.
Note that this PR needs to be rebased because CI configuration has changed after it was submitted.
Use --lock-write=lock.json or --lock-check=lock.json on the command line.
Use --lock-write=lock.json or --lock-check=lock.json on the command line.
Use --lock-write=lock.json or --lock-check=lock.json on the command line.
Use --lock-write=lock.json or --lock-check=lock.json on the command line.
Use --lock-write=lock.json or --lock-check=lock.json on the command line.
Use --lock-write=lock.json or --lock-check=lock.json on the command line.
Use --lock-write=lock.json or --lock-check=lock.json on the command line.
Use --lock-write=lock.json or --lock-check=lock.json on the command line.
Use --lock-write=lock.json or --lock-check=lock.json on the command line.
Fixes #200