esm.sh icon indicating copy to clipboard operation
esm.sh copied to clipboard

CLI init creates `deno.json` instead of updating `deno.jsonc`

Open farsightsoftware opened this issue 2 years ago • 9 comments

In a Deno project using a deno.jsonc file, I ran deno run -A -r https://esm.sh init as shown in the CLI docs. Instead of updating deno.jsonc, it created a new deno.json file (which made the project non-functional, as Deno started using the deno.json instead of deno.jsonc file).

It should update the existing deno.jsonc file instead (without removing comments). Or, alternatively, it could bail and tell the user what to add to the tasks section.

I've found the jsonc-parser package on npm that can both parse and modify JSONC, preserving comments, and I've verified it works in Deno using an npm module specifier. Here's a proof-of-concept:

import { modify, FormattingOptions } from "npm:jsonc-parser";

// Stand-in for the contents of `deno.jsonc`
let fileContents = `{
    "tasks": {
        // This is a nifty task
        "dev": "do something",
        // This is another nifty task
        "start": "do something"
    }
}`;

// In a PR, I'd derive the `tabSize` and `insertSpaces` settings from the `deno.jsonc` file contents
const formattingOptions: FormattingOptions = {
    insertSpaces: true,
    tabSize: 4,
    eol: "\n",
    keepLines: true,
};

// Add our three tasks
fileContents = addActionTask(fileContents, "add");
fileContents = addActionTask(fileContents, "update");
fileContents = addActionTask(fileContents, "remove");
console.log(fileContents);

function addActionTask(jsonc: string, action: string) {
    const taskName = `esm:${action}`;
    const taskValue = `deno run -A https://esm.sh/v128 ${action}`;
    const edits = modify(fileContents, ["tasks", taskName], taskValue, {
        formattingOptions,
    });
    for (let i = edits.length - 1; i >= 0; i--) {
        const edit = edits[i];
        jsonc =
            jsonc.substring(0, edit.offset) +
            edit.content +
            jsonc.substring(edit.offset + edit.length);
    }
    return jsonc;
}

That results in this:

{
    "tasks": {
        // This is a nifty task
        "dev": "do something",
        // This is another nifty task
        "start": "do something",
        "esm:add": "deno run -A https://esm.sh/v128 add",
        "esm:update": "deno run -A https://esm.sh/v128 update",
        "esm:remove": "deno run -A https://esm.sh/v128 remove"
    }
}

If I have time, would a PR for supporting deno.jsonc be welcome?

farsightsoftware avatar Jul 20 '23 08:07 farsightsoftware

@farsightsoftware thanks, yeah we should support deon.jsonc! and PR welcome!

ije avatar Jul 20 '23 08:07 ije

@ije Cool, thanks. Possibly st00pid question but I've never done anything with npmjs (just used the results): In the setup instructions in CONTRIBUTING.md it shows a placeholder for an access token. I've signed up for an account on npmjs and am in the access tokens section. Do I want a new "granular" token or "classic" token?

farsightsoftware avatar Jul 20 '23 08:07 farsightsoftware

it shows a placeholder for an access token

you don't need it actually.

ije avatar Jul 20 '23 08:07 ije

@ije Trying to get the tests running as a baseline before making any changes, but I'm not having much luck. CONTRIBUTING.md says to run ./test/bootstrap.sh but there isn't one. There is a ./test/bootstrap.ts, and separately as there are tests in ./tests I figured deno test would be the way to go. Running deno test did a lot of seemingly-successful tests and then failed with 429s citing error 1015 (looks like cloudflare rate limiting). Running ./test/bootstrap.ts did much the same.

Since I'm just doing CLI stuff, I tried commenting out the parts of benchmark.ts that do building tests and just leaving the runCliTest stuff. Doing that also hit some rate limits, so I left it for a bit. Coming back to it, I get this:

$ deno run -A ./test/bootstrap.ts --clean
Cleaning up...
Starting esm.sh server...
esm.sh server started.

[test CLI]
2023/07/20 16:09:26 [error] build 'v0/[email protected]/raw/package.json': npm: package 'swr' not found
2023/07/20 16:09:26 [error] build 'stable/[email protected]/raw/package.json': npm: package 'preact' not found
2023/07/20 16:09:26 [error] build 'v0/[email protected]/raw/package.json': npm: package 'preact-render-to-string' not found
error: Failed to fetch "[email protected]"
error: Failed to fetch "[email protected]"
Fail to install package: npm: package 'swr' not found
Closing esm.sh server...

Am I doing something wrong? TIA

farsightsoftware avatar Jul 20 '23 15:07 farsightsoftware

@ije - Just checking back on the above. Thanks.

farsightsoftware avatar Sep 25 '23 09:09 farsightsoftware

@farsightsoftware I'm an interested spectator here, and it seems your proposed route will be faster to do than adding a JSONC AST parser to deno std itself (which I am interested in researching and/or attempting: https://github.com/denoland/deno_std/issues/3502)

I think it would help the devs to post a draft PR towards diagnosing the concerns that you're encountering.

mmuddasani avatar Sep 25 '23 16:09 mmuddasani

@mmuddasani - I'm sorry, I don't understand what you mean by "a draft PR towards diagnosing the concerns that you're encountering". I think this issue is quite clear. The only holdup is that when I tried to start working with the project to do the PR I described, I ran into issues running the project's tests and am awaiting a response on that.

farsightsoftware avatar Sep 25 '23 16:09 farsightsoftware

@mmuddasani - I'm sorry, I don't understand what you mean by "a draft PR towards diagnosing the concerns that you're encountering".

@farsightsoftware I had misunderstood the above conversation, apologies. After reading more carefully, I also tried following the testing process, and I also encountered roadblocks.

I didn't reproduce the exact error you encountered, but I did encounter a different one (reproducible via ./test/bootstrap.ts --clean issue-502). I couldn't find an existing issue about it, so I'll open up a new one for it.

mmuddasani avatar Sep 25 '23 21:09 mmuddasani