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

Emit deprecation warnings for legacy JS API #331

Merged
merged 4 commits into from
Sep 17, 2024
Merged

Emit deprecation warnings for legacy JS API #331

merged 4 commits into from
Sep 17, 2024

Conversation

jathak
Copy link
Member

@jathak jathak commented Sep 13, 2024

Copy link
Contributor

@nex3 nex3 left a comment

Choose a reason for hiding this comment

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

Per our offline conversation, I think the best available solution for value API deprecations is to store a set of all active deprecation options and decide what to do based on those.

/**
* Shorthand for the subset of options related to deprecations.
*/
export type DeprecationOptions = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using Pick<> to derive this from the options type.


/**
* Handles a host-side deprecation warning, either emitting a warning, throwing
* and error, or doing nothing depending on the deprecation options used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* and error, or doing nothing depending on the deprecation options used.
* an error, or doing nothing depending on the deprecation options used.

@jathak jathak requested a review from nex3 September 16, 2024 22:38
* This is used to determine which options to use when handling host-side
* deprecation warnings that aren't explicitly tied to a particular compilation.
*/
export const activeDeprecationOptions: Map<Object, DeprecationOptions> =
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're using Symbols as the key here, you could actually just use a Record<Symbol, DeprecationOptions>. Either way, the key type should be Symbol.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the key type to Symbol, but keeping this as a Map because it's better for adding/removing and checking the size

*/
export function warnForHostSideDeprecation(
message: string,
deprecation: Deprecation,
options?: DeprecationOptions
): void {
if (options) throw Error(message);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this line. Is it left over from debugging?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, though I'd removed it before requesting review, so I think this was just GitHub's review interface being bad

}
return false;
}
return getDeprecationIds(options?.silenceDeprecations ?? []).includes(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return getDeprecationIds(options?.silenceDeprecations ?? []).includes(
return getDeprecationIds(options.silenceDeprecations ?? []).includes(

There's no way for options to be undefined here. Also below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@jathak jathak requested a review from nex3 September 17, 2024 18:57
@jathak jathak merged commit c4824db into main Sep 17, 2024
16 checks passed
@jathak jathak deleted the deprecate-legacy branch September 17, 2024 22:41
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.

2 participants