-
Notifications
You must be signed in to change notification settings - Fork 3.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
Clarify deprecation messages for default parameters #2802
Clarify deprecation messages for default parameters #2802
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Users are confused about why they are receiving warnings for parameters they didn't manually set
Isn't that a bug? I think we should check if it was assigned before warning.
We want to alert people who don't set it but still want it that it is deprecated and let them know they should use the extension. I'm not sure how much time should pass before we change the default to false for those settings but it should probably be soon. This will be a breaking change of course so we will need to release marked v6 |
The message is still confusing to me. The beginning "should not be used and will be removed in the future" seems contradictory to he second part "can be manually disabled". I think anyone getting this warning today is going to immediately set Maybe we should create an upgrade guide and have the warning print a link to it. |
I'm not sure I understand your point here. Mangle by default is "true". However, is is being deprecated, so users need to manually disable it (or use the separate extension) if they want the deprecation message to go away.
No, setting |
Does using the extension automatically disable it? |
Ah, probably not. I guess they need to disable the Mangle option either way to get rid of the warning. Then if they still want the mangle functionality, they should get the extension. |
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 see. That's what I thought. So the message is not very clear. Let's update the message to make this clear 👍
src/helpers.js
Outdated
@@ -250,7 +250,7 @@ export function checkDeprecations(opt, callback) { | |||
} | |||
|
|||
if (opt.mangle) { | |||
console.warn('marked(): mangle parameter is deprecated since version 5.0.0, should not be used and will be removed in the future. Instead use https://www.npmjs.com/package/marked-mangle.'); | |||
console.warn('marked(): mangle parameter is deprecated since version 5.0.0, should not be used and will be removed in the future. Note the "mangle" parameter is historically enabled by default, but can be manually disabled, or instead use https://www.npmjs.com/package/marked-mangle.'); |
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.
console.warn('marked(): mangle parameter is deprecated since version 5.0.0, should not be used and will be removed in the future. Note the "mangle" parameter is historically enabled by default, but can be manually disabled, or instead use https://www.npmjs.com/package/marked-mangle.'); | |
console.warn('marked(): mangle parameter is deprecated since version 5.0.0, and will be removed in the future. To enable, install https://npmjs.com/marked-mangle. To disable, set `{mangle: false}`.'); |
src/helpers.js
Outdated
@@ -266,7 +266,7 @@ export function checkDeprecations(opt, callback) { | |||
} | |||
|
|||
if (opt.headerIds || opt.headerPrefix) { | |||
console.warn('marked(): headerIds and headerPrefix parameters are deprecated since version 5.0.0, should not be used and will be removed in the future. Instead use https://www.npmjs.com/package/marked-gfm-heading-id.'); | |||
console.warn('marked(): headerIds and headerPrefix parameters are deprecated since version 5.0.0, should not be used and will be removed in the future. Note the "headerIds" parameter is historically enabled by default, but can be manually disabled, or instead use https://www.npmjs.com/package/marked-gfm-heading-id.'); |
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.
console.warn('marked(): headerIds and headerPrefix parameters are deprecated since version 5.0.0, should not be used and will be removed in the future. Note the "headerIds" parameter is historically enabled by default, but can be manually disabled, or instead use https://www.npmjs.com/package/marked-gfm-heading-id.'); | |
console.warn('marked(): headerIds and headerPrefix parameters are deprecated since version 5.0.0, and will be removed in the future. To enable, install https://npmjs.com/marked-gfm-heading-id. To disable, set `{headerIds: false}`.'); |
The mangle extension does disable it. |
The header id extension doesn't but I will fix that tonight. |
I fixed the header-ids extension. 👍 |
Alright. So if the extension already disables the option, is the wording clear as-is, @styfle? Or does that influence your suggested change? |
@calculuschild I updated my suggestions |
My main goal with this PR was to alert users that these options are enabled by default, to address the confusion of "Users are confused about why they are receiving warnings for parameters they didn't manually set;" So something to that effect needs to remain in the message. I will take your suggestions, @styfle, and tweak to reintroduce the mention of "enabled by default" so this can still solve the issue it was intended to. |
We need to make sure the message is actionable. Alerting for the sake of alerting is going to cause more pain for users. |
Right. Actionable, but also explain why the action is needed, which is the root of the user complaints. We can do both. Edit: @styfle , I have rewritten the messages to include both the explicit |
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.
Perfect, thanks! 🥇
## [5.0.2](v5.0.1...v5.0.2) (2023-05-11) ### Bug Fixes * Clarify deprecation messages for default parameters ([#2802](#2802)) ([763e9de](763e9de))
Marked version: 5.0.1
Markdown flavor: n/a
Description
Adds a few clarifying words to the deprecation messages for those parameters that are enabled by default. Users are confused about why they are receiving warnings for parameters they didn't manually set; this help indicate that they are enabled by default and they can also just disable the setting if they don't want to get the extension.
I could see changing "historically" to "currently" (or just removing), if that is more clear wording.
Contributor
Committer
In most cases, this should be a different person than the contributor.