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 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions cli/tests/integration/run_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4939,3 +4939,10 @@ itest!(unstable_temporal_api_missing_flag {
http_server: false,
exit_code: 1,
});

itest!(warn_on_deprecated_api {
args: "run -A run/warn_on_deprecated_api/main.js",
output: "run/warn_on_deprecated_api/main.out",
http_server: true,
exit_code: 0,
});
32 changes: 32 additions & 0 deletions cli/tests/testdata/run/warn_on_deprecated_api/main.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { runEcho as runEcho2 } from "http://localhost:4545/run/warn_on_deprecated_api/mod.ts";

const p = Deno.run({
cmd: [
Deno.execPath(),
"eval",
"console.log('hello world')",
],
});
await p.status();
p.close();

async function runEcho() {
const p = Deno.run({
cmd: [
Deno.execPath(),
"eval",
"console.log('hello world')",
],
});
await p.status();
p.close();
}

await runEcho();
await runEcho();

for (let i = 0; i < 10; i++) {
await runEcho();
}

await runEcho2();
72 changes: 72 additions & 0 deletions cli/tests/testdata/run/warn_on_deprecated_api/main.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
Download http://localhost:4545/run/warn_on_deprecated_api/mod.ts
Warning
├ Use of deprecated "Deno.run()" API.
├ This API will be removed in Deno 2.0. Make sure to upgrade to a stable API before then.
├ Suggestion: Use "Deno.Command()" API instead.
└ Stack trace:
└─ at [WILDCARD]warn_on_deprecated_api/main.js:3:16

hello world
Warning
├ Use of deprecated "Deno.run()" API.
├ This API will be removed in Deno 2.0. Make sure to upgrade to a stable API before then.
├ Suggestion: Use "Deno.Command()" API instead.
└ Stack trace:
├─ at runEcho ([WILDCARD]warn_on_deprecated_api/main.js:14:18)
└─ at [WILDCARD]warn_on_deprecated_api/main.js:25:7

hello world
Warning
├ Use of deprecated "Deno.run()" API.
├ This API will be removed in Deno 2.0. Make sure to upgrade to a stable API before then.
├ Suggestion: Use "Deno.Command()" API instead.
└ Stack trace:
├─ at runEcho ([WILDCARD]warn_on_deprecated_api/main.js:14:18)
└─ at [WILDCARD]warn_on_deprecated_api/main.js:26:7

hello world
Warning
├ Use of deprecated "Deno.run()" API.
├ This API will be removed in Deno 2.0. Make sure to upgrade to a stable API before then.
├ Suggestion: Use "Deno.Command()" API instead.
└ Stack trace:
├─ at runEcho ([WILDCARD]warn_on_deprecated_api/main.js:14:18)
└─ at [WILDCARD]warn_on_deprecated_api/main.js:29:9

hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
hello world
Warning
├ Use of deprecated "Deno.run()" API.
├ This API will be removed in Deno 2.0. Make sure to upgrade to a stable API before then.
├ Suggestion: Use "Deno.Command()" API instead.
├ Suggestion: It appears this API is used by a remote dependency.
│ Try upgrading to the latest version of that dependency.
└ Stack trace:
├─ at runEcho (http://localhost:4545/run/warn_on_deprecated_api/mod.ts:2:18)
└─ at [WILDCARD]warn_on_deprecated_api/main.js:32:7

hello world
11 changes: 11 additions & 0 deletions cli/tests/testdata/run/warn_on_deprecated_api/mod.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export async function runEcho() {
const p = Deno.run({
cmd: [
Deno.execPath(),
"eval",
"console.log('hello world')",
],
});
await p.status();
p.close();
}
5 changes: 5 additions & 0 deletions runtime/js/40_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,11 @@ function run({
...new SafeArrayIterator(ArrayPrototypeSlice(cmd, 1)),
];
}
internals.warnOnDeprecatedApi(
"Deno.run()",
(new Error()).stack,
`Use "Deno.Command()" API instead.`,
);
const res = opRun({
cmd: ArrayPrototypeMap(cmd, String),
cwd,
Expand Down
90 changes: 89 additions & 1 deletion runtime/js/99_main.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ const {
ArrayPrototypeFilter,
ArrayPrototypeIncludes,
ArrayPrototypeMap,
ArrayPrototypePop,
ArrayPrototypeShift,
DateNow,
Error,
ErrorPrototype,
Expand All @@ -23,6 +25,10 @@ const {
ObjectValues,
PromisePrototypeThen,
PromiseResolve,
SafeSet,
StringPrototypeIncludes,
StringPrototypeSplit,
StringPrototypeTrim,
Symbol,
SymbolIterator,
TypeError,
Expand Down Expand Up @@ -89,6 +95,88 @@ ObjectDefineProperties(Symbol, {
let windowIsClosing = false;
let globalThis_;

const ALREADY_WARNED_DEPRECATED = new SafeSet();

function warnOnDeprecatedApi(apiName, stack, suggestion) {
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
Contributor

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
Contributor

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

@bartlomieju bartlomieju Jan 15, 2024

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 (
StringPrototypeIncludes(stackLines[0], "(ext:") ||
StringPrototypeIncludes(stackLines[0], "(node:")
) {
ArrayPrototypeShift(stackLines);
} 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 (
StringPrototypeIncludes(stackLines[stackLines.length - 1], "(ext:core/")
) {
ArrayPrototypePop(stackLines);
}

let isFromRemoteDependency = false;
const firstStackLine = stackLines[0];
if (firstStackLine && !StringPrototypeIncludes(firstStackLine, "file:")) {
isFromRemoteDependency = true;
}

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

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
Contributor

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 "${apiName}" API.`,
"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;",
);
console.log("%c\u2502", "color: yellow;");
console.log(
`%c\u251c Suggestion: ${suggestion}`,
"color: yellow;",
);
if (isFromRemoteDependency) {
console.log("%c\u2502", "color: yellow;");
console.log(
`%c\u251c Suggestion: It appears this API is used by a remote dependency.`,
"color: yellow;",
);
console.log(
"%c\u2502 Try upgrading to the latest version of that dependency.",
"color: yellow;",
);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We should print the ability to disable these warnings with the DENO_NO_DEPRECATION_WARNINGS environment variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 ${
StringPrototypeTrim(stackLines[i])
}`,
"color: yellow;",
);
}
console.log();
}

function windowClose() {
if (!windowIsClosing) {
windowIsClosing = true;
Expand Down Expand Up @@ -432,7 +520,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