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

Added compat exports and extensibility #76

Merged
merged 5 commits into from
Nov 23, 2021
Merged

Added compat exports and extensibility #76

merged 5 commits into from
Nov 23, 2021

Conversation

gianniguida
Copy link
Contributor

@gianniguida gianniguida commented Nov 11, 2021

Changes proposed in this pull request:
Added compat exports and extensibility.

Reviewers should focus on:
Check that nothing is broken.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

This makes sense, but I'm curious: is there a use case for having the initialization utils (e.g. addComposerAutocomplete) exported, since functions can't be overriden?

@davwheat
Copy link
Member

davwheat commented Nov 11, 2021

I'd be a fan of exporting an object which contains these currently unexported utils, as this can be extended/overridden.

@askvortsov1
Copy link
Member

I'd be a fan of exporting an object which contains these currently unexported utils, as this can be extended/overridden.

Functions cannot be extended or overriden.

@davwheat
Copy link
Member

davwheat commented Nov 11, 2021

@askvortsov1 They can when part of an object. It's what we do in DiscussionControls, isn't it?

https://github.com/flarum/core/blob/master/js/src/forum/utils/DiscussionControls.js

@askvortsov1
Copy link
Member

@askvortsov1 They can when part of an object. It's what we do in DiscussionControls, isn't it?

https://github.com/flarum/core/blob/master/js/src/forum/utils/DiscussionControls.js

The functions themselves are not changed. What happens is that when we call DiscussionControls.controls(discussion), the value of DiscussionControls.controls is resolved dynamically at runtime. This means that if other code overrides DiscussionControls['controls'] at some point during boot, the DiscussionControls.controls(discussion) call will resolve to the new (overriden, in our terms) value. But the actual function has not been changed.

So, if a function is imported as a function, not dynamically resolved as a member of an object, there is no way to "override" that function. If we were to extend or override the function in compat, we would be changing compat, but not the function itself.

@iPurpl3x
Copy link
Contributor

This makes sense, but I'm curious: is there a use case for having the initialization utils (e.g. addComposerAutocomplete) exported, since functions can't be overriden?

@askvortsov1 this has been done here because it is done like that in flarum/tags. That is the only reason. I also don't currently see any use case for it. Please tell @gianniguida if he should remove those exports.

@askvortsov1
Copy link
Member

This makes sense, but I'm curious: is there a use case for having the initialization utils (e.g. addComposerAutocomplete) exported, since functions can't be overriden?

@askvortsov1 this has been done here because it is done like that in flarum/tags. That is the only reason. I also don't currently see any use case for it. Please tell @gianniguida if he should remove those exports.

I think I would prefer not to export those, as they definitely aren't part of the public API

@davwheat davwheat merged commit 343a03d into flarum:master Nov 23, 2021
@gianniguida gianniguida deleted the dev-extensibility branch November 25, 2021 08:13
askvortsov1 pushed a commit that referenced this pull request Mar 11, 2022
* Added extensibility

* Corrected object export

* Exported the `insertMention` util

* Return a `Promise` in the `reply` util (for extensibility)

* Removed initialization utils

Co-authored-by: Rafael Horvat <[email protected]>
askvortsov1 pushed a commit that referenced this pull request May 10, 2022
* Added extensibility

* Corrected object export

* Exported the `insertMention` util

* Return a `Promise` in the `reply` util (for extensibility)

* Removed initialization utils

Co-authored-by: Rafael Horvat <[email protected]>
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.

4 participants