refactor(unstable): deno info --json output #7417

bartossh · 2020-09-10T20:53:22Z

CLAassistant · 2020-09-10T20:53:49Z

All committers have signed the CLA.

bartossh · 2020-09-10T20:57:53Z

I will lint it asap and push fix.

bartossh · 2020-09-11T07:41:59Z

I think "file:https://[WILDCARD]/subdir/mod1.ts" key is not matching properly, I will try to figure it out but if anyone have an idea of how to fix it I will appreciate it very much. thx

cli/info.rs

bartlomieju · 2020-09-11T09:08:00Z

I think "file:https://[WILDCARD]/subdir/mod1.ts" key is not matching properly, I will try to figure it out but if anyone have an idea of how to fix it I will appreciate it very much. thx

I think the problem is that files are not ordered. See #7417 (comment)

bartossh · 2020-09-14T07:59:06Z

I have left deps field in ModuleDepInfo structure unchanged: here just in case its needed / will be needed. I marked to not serialize this field. It looks like deps is not used anywhere after this PR changes, so please let me know if I have to remove it.

bartlomieju · 2020-09-14T20:41:27Z

cli/info.rs

+#[serde(rename_all = "camelCase")]
+struct FileInfoVertex {
+ size: usize,
+ total_size: Option<usize>,


We don't show total size of vertex in terminal output and I think we should skip it in JSON as well. It's useful to have total size of all dependencies in top level structure, but calculating sizes of subdependencies should be left to implementors. @lucacasonato WDYT?

Yup I agree

@bartossh let's remove total_size then. Otherwise LGTM and we can land this PR

@bartlomieju Curious about this. I requested the original feature and noticed that the total size of each dependency was left out in the end. I thought this a useful feature to see how big your dependencies were. Of course, you can compute this manually by running deno info against each dependency but that's a bit of a pain. I agree that these should be consistent, just curious about the reasoning behind leaving out the total size of dependencies.

I have implemented the change for total_size that @bartlomieju and @lucacasonato agreed on, removing it from vertex of a flat graph, and added total_sizeto module info on the top level so only total size of module will be shown.
But if you think @cknight suggestion should be applied I will cherry pic previous implementation and push changes back again.

this will look like this

Thanks

I agree that these should be consistent, just curious about the reasoning behind leaving out the total size of dependencies.

Circular dependencies can be really confusing, especially if they show up multiple times. Showing only the size of singular dependency is what browsers do. If there will be huge demand for "total" size we can revisit the topic, but for now I think we should go only with size of the concrete module without it's deps.

Thanks @bartlomieju, makes sense

cli/info.rs

… info

bartossh · 2020-09-15T10:02:24Z

cli/info.rs

@@ -374,7 +430,7 @@ mod test {
 }

 #[test]
- fn get_new_prefix_adds_a_vertial_bar_if_not_is_last() {
+ fn get_new_prefix_adds_a_vertical_bar_if_not_is_last() {


spelling correction , not part of this feature

bartlomieju

LGTM, thank you @bartossh

bartlomieju · 2020-09-15T10:04:58Z

@bartossh could you fix #7491 in this PR as well? That should be a minimal change

…at_graph

…into deno_info_json_flat_graph

bartossh · 2020-09-15T11:58:20Z

After change It isn't causing the issue but not all dependencies are printed:

local: /home/bartossh/.cache/deno/deps/https/deno.land/46b808b8747ed75c4be741a37f6b10130ff7de51f37b58ee9b920d0a76aa6cf9
type: TypeScript
deps: 12 unique (total 63.31KB)
https://deno.land/[email protected]/http/server.ts (10.23KB)
├── https://deno.land/[email protected]/encoding/utf8.ts (433B)
├─┬ https://deno.land/[email protected]/io/bufio.ts (21.15KB)
│ ├── https://deno.land/[email protected]/bytes/mod.ts (4.34KB)
│ └── https://deno.land/[email protected]/_util/assert.ts (405B)
├── https://deno.land/[email protected]/_util/assert.ts *
├─┬ https://deno.land/[email protected]/async/mod.ts (202B)

It contains cyclic dependencies https://deno.land/std/http/server.ts redirected to https://deno.land/[email protected]/http/server.ts, so I suppose this should to be ended like so.

bartossh · 2020-09-15T18:43:46Z

If submodule that main module is linking to contains cyclic dependencies, maybe the best way to handle the issue of not going in to a loop, is to return some informative message to the user and then end the print of a dependency tree in some nice way?

Shall We open an issue for this and sort it out in a new PR or shall I sort it out in this PR?

bartlomieju · 2020-09-15T18:47:49Z

If submodule that main module is linking to contains cyclic dependencies, maybe the best way to handle the issue of not going in to a loop, is to return some informative message to the user and then end the print of a dependency tree in some nice way?

@bartossh this has already been handled in #6786

bartossh · 2020-09-15T19:05:12Z

Yup. I wrote it being not specific enough. Sorry and thanks @bartlomieju for pointing this out for me with related PR example. As you see in the above example the dependency tree finishes in an ugly way. I can manage to solve this. I just have no idea what is the expected outcome.

bartlomieju · 2020-09-15T19:07:41Z

Yup. I wrote it being not specific enough. Sorry and thanks @bartlomieju for pointing this out for me with related PR example. As you see in the above example the dependency tree finishes in an ugly way. I can manage to solve this. I just have no idea what is the expected outcome.

@bartossh it's unexpected that it stopped working - I have no problem to see whole dep tree in 1.4.0. I will investigate tomorrow.

bartossh · 2020-09-16T06:48:23Z

@bartossh it's unexpected that it stopped working - I have no problem to see whole dep tree in 1.4.0. I will investigate tomorrow.

@bartlomieju it is my mistake. In the last example I have blindly copy pasted the command with | head, getting last 10 lines. 🤦 , It looks alright:

./deno info https://deno.land/[email protected]/http/server.ts
local: /home/bartossh/.cache/deno/deps/https/deno.land/46b808b8747ed75c4be741a37f6b10130ff7de51f37b58ee9b920d0a76aa6cf9
type: TypeScript
deps: 12 unique (total 63.31KB)
https://deno.land/[email protected]/http/server.ts (10.23KB)
├── https://deno.land/[email protected]/encoding/utf8.ts (433B)
├─┬ https://deno.land/[email protected]/io/bufio.ts (21.15KB)
│ ├── https://deno.land/[email protected]/bytes/mod.ts (4.34KB)
│ └── https://deno.land/[email protected]/_util/assert.ts (405B)
├── https://deno.land/[email protected]/_util/assert.ts *
├─┬ https://deno.land/[email protected]/async/mod.ts (202B)
│ ├── https://deno.land/[email protected]/async/deferred.ts (1.03KB)
│ ├── https://deno.land/[email protected]/async/delay.ts (279B)
│ ├─┬ https://deno.land/[email protected]/async/mux_async_iterator.ts (1.98KB)
│ │ └── https://deno.land/[email protected]/async/deferred.ts *
│ └── https://deno.land/[email protected]/async/pool.ts (1.58KB)
└─┬ https://deno.land/[email protected]/http/_io.ts (11.25KB)
  ├── https://deno.land/[email protected]/io/bufio.ts *
  ├─┬ https://deno.land/[email protected]/textproto/mod.ts (4.52KB)
  │ ├── https://deno.land/[email protected]/io/bufio.ts *
  │ ├── https://deno.land/[email protected]/bytes/mod.ts *
  │ └── https://deno.land/[email protected]/encoding/utf8.ts *
  ├── https://deno.land/[email protected]/_util/assert.ts *
  ├── https://deno.land/[email protected]/encoding/utf8.ts *
  ├── https://deno.land/[email protected]/http/server.ts *
  └── https://deno.land/[email protected]/http/http_status.ts (5.93KB)

Sorry for the confusion.

bartlomieju · 2020-09-16T09:01:01Z

@bartossh no worries, glad it's not broken after all. I will review and land your PR today.

bartlomieju

LGTM, thank you @bartossh

bartossh added 3 commits September 10, 2020 18:46

flatten nested json to flat graph json

8e42efe

update test info file json structure

bfe8be8

WILDCARD

051f204

linted

682b7ae

bartlomieju reviewed Sep 11, 2020

View reviewed changes

cli/info.rs Outdated Show resolved Hide resolved

bartossh added 2 commits September 13, 2020 13:25

use module graph and sort serialized output by the key

febe905

lint corrections

14bf3fc

bartossh requested a review from bartlomieju September 13, 2020 12:31

bartossh added 2 commits September 13, 2020 15:02

simplify code with unwraping

e037529

avoid to follow cyclic dependencies

3d78c1a

bartlomieju reviewed Sep 14, 2020

View reviewed changes

bartlomieju changed the title ~~deno info json flat graph~~ refactor(unstable): deno info --json output Sep 14, 2020

lucacasonato reviewed Sep 14, 2020

View reviewed changes

cli/info.rs Outdated Show resolved Hide resolved

remove total_size from all graph vertexes and add show only in module…

7b0fe3b

… info

bartossh commented Sep 15, 2020

View reviewed changes

Merge branch 'master' into deno_info_json_flat_graph

5ae2413

bartlomieju approved these changes Sep 15, 2020

View reviewed changes

bartlomieju and others added 6 commits September 15, 2020 12:06

fmt

2ef9adf

spelling

e5076c0

Merge remote-tracking branch 'upstream/master' into deno_info_json_fl…

3e12f8b

…at_graph

correct ErrBox useage

97e5938

Merge branch 'deno_info_json_flat_graph' of github.com:bartOssh/deno …

6852111

…into deno_info_json_flat_graph

write all to stdout

a5c3032

bartossh requested review from bartlomieju and lucacasonato September 15, 2020 12:12

ignore broken pipe

ea4870f

bartlomieju approved these changes Sep 16, 2020

View reviewed changes

bartlomieju added 3 commits September 16, 2020 12:27

Merge branch 'master' into deno_info_json_flat_graph

83271fe

Merge branch 'master' into deno_info_json_flat_graph

636de26

reset CI

11767b2

bartlomieju merged commit 81ca709 into denoland:master Sep 16, 2020

bartossh deleted the deno_info_json_flat_graph branch September 16, 2020 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(unstable): deno info --json output #7417

refactor(unstable): deno info --json output #7417

bartossh commented Sep 10, 2020 •

edited by bartlomieju

Loading

CLAassistant commented Sep 10, 2020 •

edited

Loading

bartossh commented Sep 10, 2020 •

edited

Loading

bartossh commented Sep 11, 2020

bartlomieju commented Sep 11, 2020

bartossh commented Sep 14, 2020

bartlomieju Sep 14, 2020

lucacasonato Sep 14, 2020

bartlomieju Sep 15, 2020

cknight Sep 15, 2020

bartossh Sep 15, 2020 •

edited

Loading

bartlomieju Sep 15, 2020

cknight Sep 15, 2020

bartossh Sep 15, 2020

bartlomieju left a comment

bartlomieju commented Sep 15, 2020

bartossh commented Sep 15, 2020 •

edited

Loading

bartossh commented Sep 15, 2020

bartlomieju commented Sep 15, 2020

bartossh commented Sep 15, 2020

bartlomieju commented Sep 15, 2020

bartossh commented Sep 16, 2020

bartlomieju commented Sep 16, 2020

bartlomieju left a comment

refactor(unstable): deno info --json output #7417

refactor(unstable): deno info --json output #7417

Conversation

bartossh commented Sep 10, 2020 • edited by bartlomieju Loading

CLAassistant commented Sep 10, 2020 • edited Loading

bartossh commented Sep 10, 2020 • edited Loading

bartossh commented Sep 11, 2020

bartlomieju commented Sep 11, 2020

bartossh commented Sep 14, 2020

bartlomieju Sep 14, 2020

Choose a reason for hiding this comment

lucacasonato Sep 14, 2020

Choose a reason for hiding this comment

bartlomieju Sep 15, 2020

Choose a reason for hiding this comment

cknight Sep 15, 2020

Choose a reason for hiding this comment

bartossh Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

bartlomieju Sep 15, 2020

Choose a reason for hiding this comment

cknight Sep 15, 2020

Choose a reason for hiding this comment

bartossh Sep 15, 2020

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

bartlomieju commented Sep 15, 2020

bartossh commented Sep 15, 2020 • edited Loading

bartossh commented Sep 15, 2020

bartlomieju commented Sep 15, 2020

bartossh commented Sep 15, 2020

bartlomieju commented Sep 15, 2020

bartossh commented Sep 16, 2020

bartlomieju commented Sep 16, 2020

bartlomieju left a comment

Choose a reason for hiding this comment

bartossh commented Sep 10, 2020 •

edited by bartlomieju

Loading

CLAassistant commented Sep 10, 2020 •

edited

Loading

bartossh commented Sep 10, 2020 •

edited

Loading

bartossh Sep 15, 2020 •

edited

Loading

bartossh commented Sep 15, 2020 •

edited

Loading