Skip to content

feat: deprecate custom decorators#1587

Closed
nknapp wants to merge 1 commit into4.xfrom
deprecate-decorators
Closed

feat: deprecate custom decorators#1587
nknapp wants to merge 1 commit into4.xfrom
deprecate-decorators

Conversation

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 28, 2019

This PR marks the "registerDecorator" is now deprecated. The
decorator API exposes a lot of internal structures that
should not be part of the official API. An alternative
API can be discussed at #1574

  • "Handlebars.registerDecorator" now logs a warning
  • "Handlebars.registerDecoratorWithoutDeprecationWarning"
    can be used in order to omit the warning, when
    developers have read the notice.
    (The inline-decorator is using this function now)

I would value comments about whether this is the correct way to introduce a deprecation.
My thoughts:

  • Pro: People only read a changelog when they have build failures. A deprecation notice in the build
    log might be more visible than an entry in the release-notes
  • Contra: The warning will probably appear in the browser-console of end-users. This may not be
    what developers want. In some cases, developer my have a hard time removing the warnings,
    because they don't have direct access to the code that uses Handlebars. (like this comment suggests)
  • Contra: The message may not be as visible as I intend, if it is just in the build log and in the browser-console.

the function "registerDecorator" is now deprecated. The
decorator API exposes a lot of internal structures that
should not be part of the official API. An alternative
API can be discussed at #1574
- "Handlebars.registerDecorator" now logs a warning
- "Handlebars.registerDecoratorWithoutDeprecationWarning"
  can be used in order to omit the warning, when
  developers have read the notice.
  (The inline-decorator is using this function now)
@nknapp nknapp requested review from ErisDS and wycats October 28, 2019 19:26
@nknapp
Copy link
Collaborator Author

nknapp commented Oct 28, 2019

@wycats, @ErisDS opinions? I will fix the tests before merging, but I would like to know if what I am doing makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant