-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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: Add warnings for more deprecated APIs #21992
feat: Add warnings for more deprecated APIs #21992
Conversation
Signed-off-by: Asher Gomez <ashersaupingomez@gmail.com>
Note: |
ext/io/12_io.js
Outdated
internals.warnOnDeprecatedApi( | ||
"Deno.iter()", | ||
new Error().stack, | ||
"Use `ReadableStreamDefaultReader` instead.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err I don't understand this suggestion :( don't we have iter()
equivalent in deno_std
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's deprecated. Should we add it back in? I was wondering whether we should add it back in...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not. Maybe we shouldn't add a suggestion here - this API relies on Reader
interface that is also deprecated. So migrating the underlying Reader
dictates which API should be used instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Reader
interface in deno_std
is no longer deprecated, so this might be fine to add back in.
Also, actually, should it not be ReadableStream
instead? Both ReadableStream
and Deno.iter()
are async iterable.
We can either add iter()
back to deno_std
or suggest ReadableStream
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's suggest ReadableStream
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realise doing this all in runtime/js/90_deno_ns.js
will be cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Follow up to #21939 that adds deprecation
warnings to more
Deno
APIs:Deno.copy()
Deno.iter()
Deno.iterSync()
new Deno.Buffer()
Deno.readAll()
Deno.readAllSync()
Deno.writeAll()
Deno.writeAllSync()
Deno.FsWatcher.return
Deno.serveHttp()
Deno.metrics()