-
-
Notifications
You must be signed in to change notification settings - Fork 835
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
Deprecate unused evented
utility
#3125
Conversation
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.
Excellent, thank you very much! Aside from the suggestion in the code, the only minor idea I have is changing the wording to "the evented
util is deprecated and will be removed in v2.0 of Flarum". Otherwise, LGTM!
As to other steps, no bundled extensions currently use this util, so no action is needed from the Flarum side, so this issue is effectively done with this PR. Thank you very much for the contribution!
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'm hesitant towards this, personally.
I think we should use the existing debug warning helper we have, instead, to prevent firing unnecessary console messages in production forums: https://github.com/flarum/core/blob/master/js/src/common/helpers/fireDebugWarning.ts
Furthermore, I'd prefer us to use a more descriptive message for this and to store it in a variable rather than repeating it.
Maybe...
"The `evented` util is deprecated and will be removed in Flarum 2.0. For more info, please see https://github.com/flarum/core/issues/2547"
I know very few or no extensions actually use this, but it sets a precedent for future deprecations. (And is very similar to the tootips message.)
To be clear, there's nothing inherently wrong with this PR, and a big thank you goes out to @fredden. First time PRs are nerve-wracking, and I know that from experience! ❤️
Thanks for the feedback. I'll make the suggested changes and ask for another review. |
This is related to #2547, but is only step one of two listed there
Changes proposed in this pull request:
I found #2547 while looking for a task to get started with. This is my first contribution to this project.
The issue mentions marking
evented
as deprecated and removing this. This pull request marks that utility as deprecated so we can safely remove the same in the future. This pull request introduces no functional changes.Reviewers should focus on:
I'm unsure what to suggest here as the changes shouldn't have any impact on existing functionality, but is instead future-planning.
Necessity
Confirmed
composer test
).Required changes:
evented
in https://github.com/flarum/docs/search?q=evented