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

Lockfile support for experimental npm specifiers #15696

Closed
sagea opened this issue Aug 30, 2022 · 3 comments · Fixed by #15938
Closed

Lockfile support for experimental npm specifiers #15696

sagea opened this issue Aug 30, 2022 · 3 comments · Fixed by #15938

Comments

@sagea
Copy link
Contributor

sagea commented Aug 30, 2022

Bug:
Lock files aren't supported when using the new experimental npm modules feature.

Why post this?
The release notes below describe the list of specifiers that are supported and those that aren't. Deno cache is not in that list as of writing this ticket.
https://deno.com/blog/v1.25#experimental-npm-support

Repro Steps

  1. Create a new folder
mkdir deno-npm-lock-repro
  1. cd into folder
cd deno-npm-lock-repro
  1. Create a new project
deno init
  1. Create an express app in the main.ts file
import express from "npm:express";
const app = express();

app.get("/", function (req, res) {
  res.send("Hello World");
});

app.listen(3000);
console.log("listening on http://localhost:3000/");
  1. Create a lock file using the deno cache command
deno cache --reload --lock=lock.json --lock-write --unstable main.ts

EXPECTED:
A file is created named lock.json with a list of modules.

ACTUAL:
A file is created named lock.json but it has an empty object.

@sagea sagea changed the title [BUG] Deno 1.25 npm experimental support: Lock files aren't populated [BUG] Deno 1.25 npm experimental support: Lock files aren't being populated Aug 30, 2022
@kitsonk kitsonk added bug Something isn't working correctly node compat labels Aug 30, 2022
@bartlomieju
Copy link
Member

bartlomieju commented Aug 30, 2022

Yes, lock file integration with npm: specifiers is currently not supported. This is on the radar for @dsherret and me

@bartlomieju
Copy link
Member

Ignore PR linked above, I assigned wrong issue number to close.

@dsherret dsherret removed the bug Something isn't working correctly label Aug 31, 2022
@dsherret dsherret changed the title [BUG] Deno 1.25 npm experimental support: Lock files aren't being populated Lockfile support for experimental npm specifiers Aug 31, 2022
@dsherret dsherret mentioned this issue Sep 19, 2022
25 tasks
@bartlomieju
Copy link
Member

bartlomieju commented Oct 18, 2022

Supporting npm packages in lock file requires us to introduce a new lock file format, because the current lockfile has only a top-level mapping between URLs and their checksums. In case of npm specifier, not only do we need to store the checksum, but the resolution result of imports (eg. import chalk from "npm:chalk" resolves to chalk@5.0.1 and import react from "npm:react@17" resolves to react@17.0.1`).

So to achieve this goal I propose following file format:

// This file is automatically generated by Deno, do not edit its contents
// manually.
// This file should be commited to your repository.
{
  "version": "2",
  "dependencies": {
    "remote": {
        "https://deno.land/std@0.160.0/http/server.ts": "asdwetsw44523asdfgfas..",
        "https://deno.land/std@0.160.0/http/file_server.ts": "asdwetsw44523asdfgfas..",
    },
    "npm": {
        "chalk@5.0.0": "sha256-asdf4356tsdfgww345",
        "react@17.0.1": "sha256-sadfwer334563456"
    }
  },
  "resolution": {
    "chalk": {
        "major": 5,
        "minor": 0,
        "patch": 0,
        "pre": [],
        "build": []
    },
    "react@17": {
        "major": 17,
        "minor": 0,
        "patch": 1,
        "pre": [],
        "build": []
    }
  }
}

The file would use jsonc format and I think we should default to deno.lock filename.

It allows us to change the format in the future, while preserving compatibility with previous version (or at least provide an easy upgrade path from one version to the next); store additional required info (if a need arises in the future).

If there's an agreement regarding the format I will open a PR that will first add new format to handle remote dependencies and follow up with support for npm. In addition we'll try to address #11971 in v1.27 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants