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

fix(console): deprecate Deno.customInspect #10035

Merged
merged 9 commits into from
Jun 25, 2021

Conversation

kt3k
Copy link
Member

@kt3k kt3k commented Apr 6, 2021

This PR deprecates Deno.customInspect as suggested in #9294 and suggests using Symbol.for("Deno.customInspect") instead.

ref #9294

Comment on lines 910 to 920
return String(value[customInspect]());
}
// This non-unique symbol is used to support op_crates, ie.
// in op_crates/web we don't want to depend on unique "Deno.customInspect"
// symbol defined in the public API. Internal only, shouldn't be used
// by users.
const nonUniqueCustomInspect = Symbol.for("Deno.customInspect");
if (
nonUniqueCustomInspect in value &&
typeof value[nonUniqueCustomInspect] === "function"
) {
Copy link
Member

Choose a reason for hiding this comment

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

Removing this logic means that Deno.customInspect will stop working. I think we should still support it until 2.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

The non-unique custom inspect which we use for op crates implementations should be renamed to something else since the public custom inspect symbol has stolen its name. We handle the internal one with its own logic (i.e. pass it the inspect() function as an arg) so the difference should be preserved.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should always pass the inspect function as an arg - even for the public one.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. I'll rename internal one to something else and keep the different handlings of public one and internal one.

@bartlomieju bartlomieju added this to the 1.12.0 milestone Jun 18, 2021
@bartlomieju
Copy link
Member

@kt3k could you please update no-deprecated-deno-api lint rule (https://github.com/denoland/deno_lint/blob/main/src/rules/no_deprecated_deno_api.rs) after landing this PR?

@kt3k
Copy link
Member Author

kt3k commented Jun 22, 2021

@bartlomieju Sure. I'll try.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Lint rule has already landed in denoland/deno_lint#748 so this is ready to land. Looks good to me

@kt3k kt3k merged commit d832d2b into denoland:main Jun 25, 2021
@kt3k kt3k deleted the feat/deprecate-deno-custom-inspect branch June 25, 2021 07:19
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.

4 participants