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

feat: Start warning on each use of a deprecated API #21939

Merged
merged 17 commits into from Jan 18, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion ext/io/12_io.js
Expand Up @@ -4,7 +4,7 @@
// Documentation liberally lifted from them too.
// Thank you! We love Go! <3

import { core, primordials } from "ext:core/mod.js";
import { core, internals, primordials } from "ext:core/mod.js";
const {
op_stdin_set_raw,
} = core.ensureFastOps(true);
Expand Down Expand Up @@ -39,6 +39,7 @@ async function copy(
dst,
options,
) {
internals.warnOnDeprecatedApi("Deno.copy()", (new Error()).stack);
let n = 0;
const bufSize = options?.bufSize ?? DEFAULT_BUFFER_SIZE;
const b = new Uint8Array(bufSize);
Expand All @@ -64,6 +65,7 @@ async function* iter(
r,
options,
) {
internals.warnOnDeprecatedApi("Deno.iter()", (new Error()).stack);
const bufSize = options?.bufSize ?? DEFAULT_BUFFER_SIZE;
const b = new Uint8Array(bufSize);
while (true) {
Expand All @@ -80,6 +82,7 @@ function* iterSync(
r,
options,
) {
internals.warnOnDeprecatedApi("Deno.iterSync()", (new Error()).stack);
const bufSize = options?.bufSize ?? DEFAULT_BUFFER_SIZE;
const b = new Uint8Array(bufSize);
while (true) {
Expand Down
7 changes: 6 additions & 1 deletion runtime/js/13_buffer.js
Expand Up @@ -4,7 +4,7 @@
// Copyright 2009 The Go Authors. All rights reserved. BSD license.
// https://github.com/golang/go/blob/master/LICENSE

import { primordials } from "ext:core/mod.js";
import { internals, primordials } from "ext:core/mod.js";
const {
ArrayBufferPrototypeGetByteLength,
TypedArrayPrototypeSubarray,
Expand Down Expand Up @@ -45,6 +45,7 @@ class Buffer {
#off = 0; // read at buf[off], write at buf[buf.byteLength]

constructor(ab) {
internals.warnOnDeprecatedApi("new Deno.Buffer()", (new Error()).stack);
if (ab == null) {
this.#buf = new Uint8Array(0);
return;
Expand Down Expand Up @@ -230,25 +231,29 @@ class Buffer {
}

async function readAll(r) {
internals.warnOnDeprecatedApi("Deno.readAll()", (new Error()).stack);
const buf = new Buffer();
await buf.readFrom(r);
return buf.bytes();
}

function readAllSync(r) {
internals.warnOnDeprecatedApi("Deno.readAllSync()", (new Error()).stack);
const buf = new Buffer();
buf.readFromSync(r);
return buf.bytes();
}

async function writeAll(w, arr) {
internals.warnOnDeprecatedApi("Deno.writeAll()", (new Error()).stack);
let nwritten = 0;
while (nwritten < arr.length) {
nwritten += await w.write(TypedArrayPrototypeSubarray(arr, nwritten));
}
}

function writeAllSync(w, arr) {
internals.warnOnDeprecatedApi("Deno.writeAllSync()", (new Error()).stack);
let nwritten = 0;
while (nwritten < arr.length) {
nwritten += w.writeSync(TypedArrayPrototypeSubarray(arr, nwritten));
Expand Down
6 changes: 5 additions & 1 deletion runtime/js/40_fs_events.js
@@ -1,6 +1,6 @@
// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.

import { core, primordials } from "ext:core/mod.js";
import { core, internals, primordials } from "ext:core/mod.js";
const {
BadResourcePrototype,
InterruptedPrototype,
Expand Down Expand Up @@ -49,6 +49,10 @@ class FsWatcher {
// TODO(kt3k): This is deprecated. Will be removed in v2.0.
// See https://github.com/denoland/deno/issues/10577 for details
return(value) {
internals.warnOnDeprecatedApi(
"Deno.FsWatcher.return()",
(new Error()).stack,
);
core.close(this.rid);
return PromiseResolve({ value, done: true });
}
Expand Down
3 changes: 2 additions & 1 deletion runtime/js/40_http.js
@@ -1,12 +1,13 @@
// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
import { core } from "ext:core/mod.js";
import { core, internals } from "ext:core/mod.js";
const {
op_http_start,
} = core.ensureFastOps();

import { HttpConn } from "ext:deno_http/01_http.js";

function serveHttp(conn) {
internals.warnOnDeprecatedApi("Deno.serveHttp()", (new Error()).stack);
const rid = op_http_start(conn.rid);
return new HttpConn(rid, conn.remoteAddr, conn.localAddr);
}
Expand Down
1 change: 1 addition & 0 deletions runtime/js/40_process.js
Expand Up @@ -140,6 +140,7 @@ function run({
...new SafeArrayIterator(ArrayPrototypeSlice(cmd, 1)),
];
}
internals.warnOnDeprecatedApi("Deno.run()", (new Error()).stack);
const res = opRun({
cmd: ArrayPrototypeMap(cmd, String),
cwd,
Expand Down
7 changes: 5 additions & 2 deletions runtime/js/90_deno_ns.js
@@ -1,6 +1,6 @@
// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.

import { core } from "ext:core/mod.js";
import { core, internals } from "ext:core/mod.js";
const {
op_net_listen_udp,
op_net_listen_unixpacket,
Expand Down Expand Up @@ -31,7 +31,10 @@ import * as kv from "ext:deno_kv/01_db.ts";
import * as cron from "ext:deno_cron/01_cron.ts";

const denoNs = {
metrics: core.metrics,
metrics: () => {
internals.warnOnDeprecatedApi("Deno.metrics()", (new Error()).stack);
return core.metrics();
},
Process: process.Process,
run: process.run,
isatty: tty.isatty,
Expand Down
61 changes: 60 additions & 1 deletion runtime/js/99_main.js
Expand Up @@ -8,7 +8,9 @@ const ops = core.ops;
const {
ArrayPrototypeFilter,
ArrayPrototypeIncludes,
ArrayPrototypeJoin,
ArrayPrototypeMap,
ArrayPrototypeShift,
DateNow,
Error,
ErrorPrototype,
Expand All @@ -23,6 +25,8 @@ const {
ObjectValues,
PromisePrototypeThen,
PromiseResolve,
SafeSet,
StringPrototypeSplit,
Symbol,
SymbolIterator,
TypeError,
Expand Down Expand Up @@ -89,6 +93,61 @@ ObjectDefineProperties(Symbol, {
let windowIsClosing = false;
let globalThis_;

const ALREADY_WARNED_DEPRECATED = new SafeSet();

function warnOnDeprecatedApi(apiName, stack) {
if (ALREADY_WARNED_DEPRECATED.has(apiName + stack)) {
return;
}

// If we haven't warned yet, let's do some processing of the stack trace
// to make it more useful.
const stackLines = StringPrototypeSplit(stack, "\n");
ArrayPrototypeShift(stackLines);
Copy link
Member

Choose a reason for hiding this comment

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

Curious why we shift one item here

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is to remove the Error\n at the start of the output from new Error().stack.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if that's worth the cost of split, shift, and join

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest stack.slice(6) with a comment.

Copy link
Member

Choose a reason for hiding this comment

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

I also suggest we should do that slice at the line of "%cThis API was called from:\n" + stackString + "\n", to minimize the overhead of this util

Copy link
Member Author

Choose a reason for hiding this comment

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

This is also filtering irrelevant stack frames. Without it the stacks are very noisy and make it hard to figure out where the API is called. It's not like this API will be called in the hot path (and if it is it's yet another reason to fix user code quickly :))

Copy link
Member Author

Choose a reason for hiding this comment

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

I will change the logic to first check if such stack traces was produced before manipulating the stack. Done.

while (true) {
// Filter out internal frames at the top of the stack - they are not useful
// to the user.
if (stackLines[0].includes("(ext:") || stackLines[0].includes("(node:")) {
stackLines.shift();
} else {
break;
}
}
// Now remove the last frame if it's coming from "ext:core" - this is most likely
// event loop tick or promise handler calling a user function - again not
// useful to the user.
if (stackLines[stackLines.length - 1].includes("(ext:core/")) {
stackLines.pop();
}

ALREADY_WARNED_DEPRECATED.add(apiName + stack);
console.log(
"%cWarning",
"color: yellow; font-weight: bold;",
);
console.log(
Copy link
Collaborator

Choose a reason for hiding this comment

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

When importing from deno_std without a pinned version (e.g. import * as http from "https://deno.land/std/http/server.ts), the warning printed has a yellow "Warning" prefix and unformatted rest-of-text. Should this warning be consistent with that?
image

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I want it to stand out and be sore to the eyes :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea 😆

`%c\u251c Use of deprecated API "${apiName}".`,
"color: yellow;",
);
console.log("%c\u2502", "color: yellow;");
console.log(
"%c\u251c This API will be removed in Deno 2.0. Make sure to upgrade to a stable API before then.",
"color: yellow;",
);
// TODO(bartlomieju): add API suggestion to what to migrate to
console.log("%c\u2502", "color: yellow;");
console.log("%c\u2514 Stack trace:", "color: yellow;");
for (let i = 0; i < stackLines.length; i++) {
console.log(
`%c ${i == stackLines.length - 1 ? "\u2514" : "\u251c"}\u2500 ${
stackLines[i].trim()
}`,
"color: yellow;",
);
}
console.log();
}

function windowClose() {
if (!windowIsClosing) {
windowIsClosing = true;
Expand Down Expand Up @@ -432,7 +491,7 @@ function exposeUnstableFeaturesForWindowOrWorkerGlobalScope(options) {
// FIXME(bartlomieju): temporarily add whole `Deno.core` to
// `Deno[Deno.internal]` namespace. It should be removed and only necessary
// methods should be left there.
ObjectAssign(internals, { core });
ObjectAssign(internals, { core, warnOnDeprecatedApi });
const internalSymbol = Symbol("Deno.internal");
const finalDenoNs = {
internal: internalSymbol,
Expand Down