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

Add more console types formatting support #1299

Merged
merged 2 commits into from Dec 10, 2018

Conversation

kevinkassimo
Copy link
Contributor

@kevinkassimo kevinkassimo commented Dec 10, 2018

Noticed that we haven't yet support types such as Map when I was doing some debugging (which is annoying...).

This PR adds formatting support to the following types:

  • Set/Map
  • WeakSet/WeakMap (WeakMap { [items unknown] }, similar behavior to Node)
  • Wrapper objects (Number/Boolean/String)
  • Date
  • RegExp
  • TypedArray (Uint8Array etc.)

Changed:

  • DEFAULT_MAX_DEPTH for nested object printing is increased to 4 (2 is too restrictive)
  • [object] => [Object] for deeply nested objects (as seen in Node)
  • [object] => [Set]/[Map] etc. for deeply nested Set/Map (as seen in Node)

@kevinkassimo
Copy link
Contributor Author

kevinkassimo commented Dec 10, 2018

I am seeing errors about deno_net on AppVeyor again:

error: Server does not allow request for unadvertised object 958dadc8752f1aface8cff39c56011b016fb1460
Fetched in submodule path 'js/deps/https/deno.land/x/net', but it did not contain 958dadc8752f1aface8cff39c56011b016fb1460. Direct fetching of that commit failed.

js/console.ts Outdated
const DEFAULT_MAX_DEPTH = 2;
const DEFAULT_MAX_DEPTH = 4;

const TYPEDARRAY_TYPES = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw somewhere in another commit of deno use TypedArray, which can be exposed by doing Object.getPrototypeof(Int8Array), which all typed arrays extend. It's shorter and provides support for BigInt64Array and BigUint64Array, which the version of V8 we use supports, and possibly any other typed array that gets implemented in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Found it! It's in util.ts, we could use this function instead of running a loop for each typed array:

deno/js/util.ts

Lines 129 to 133 in c427c2d

// tslint:disable-next-line:variable-name
const TypedArrayConstructor = Object.getPrototypeOf(Uint8Array);
export function isTypedArray(x: unknown): x is TypedArray {
return x instanceof TypedArrayConstructor;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! Thanks for the suggestion. Will update right away

(Did not notice that I should import TypedArray from types.ts)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that too 👍

js/console.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

Other than this, it looks LGTM

return createWeakSetString();
} else if (value instanceof WeakMap) {
return createWeakMapString();
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

The next line could be inlined for consistency with the other conditions, using else if, and createRawObjectString inside the else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah... (I was too tired and totally ignored this...)

@ry
Copy link
Member

ry commented Dec 10, 2018

@kevinkassimo

error: Server does not allow request for unadvertised object

Thanks for the notice. Should be fixed now. It's because git submodules requires the linked revision to be published as the head of a branch (?). I'll think about how to deal with this in the future.

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.

LGTM - thanks!

@ry ry merged commit 1548792 into denoland:master Dec 10, 2018
ry added a commit to ry/deno that referenced this pull request Dec 14, 2018
- console.assert should not throw error (denoland#1335)
- Support more modes in deno.open (denoland#1282, denoland#1336)
- Simplify code fetch logic (denoland#1322)
- readDir entry mode (denoland#1326)
- Use stderr for exceptions (denoland#1303)
- console.log formatting improvements (denoland#1327, denoland#1299)
- Expose TooLarge error code for buffers (denoland#1298)
@ry ry mentioned this pull request Dec 14, 2018
ry added a commit that referenced this pull request Dec 14, 2018
- console.assert should not throw error (#1335)
- Support more modes in deno.open (#1282, #1336)
- Simplify code fetch logic (#1322)
- readDir entry mode (#1326)
- Use stderr for exceptions (#1303)
- console.log formatting improvements (#1327, #1299)
- Expose TooLarge error code for buffers (#1298)
@kevinkassimo kevinkassimo deleted the console/more-types branch December 27, 2019 07:51
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

3 participants