chore: remove frontend scoping in Modal

[DavideIadeluca]

Fixes #0000
TS will throw errors of various kinds if a third-party extension tries to customize the behaviour of existing Modals. For example, a third-party implementation might want to remove the ability to close the SignUpModal via the close button:

import SignUpModal from 'flarum/forum/components/SignUpModal';

export default function () {
    SignUpModal.isDismissibleViaCloseButton = false;
  }

This will result in the following error:

Cannot assign to 'isDismissibleViaCloseButton' because it is a read-only property.ts(2540)

Changes proposed in this pull request:

• Remove explicit scoping of properties, methods and getter in the Modal class and remove readonly modifier.

Reviewers should focus on:
This change got me thinking... Do we really need to constrain scopes in the JS of the framework, like at all? I think in the spirit of Flarum it would be a welcome change for extension developers if such scoping is removed and TS workarounds related to this were no longer required.

I would be happy to amend this PR do the change across the whole codebase

Screenshot

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.
• Backend changes: tests are green (run composer test).
• Core developer confirmed locally this works as intended.
• Tests have been added, or are not appropriate here.

Required changes:

• Related documentation PR: (Remove if irrelevant)

[SychO9]

Thanks for the PR!

Generally, it depends on the specific case. We mark certain things as protected or readonly when we believe something might change in a minor release, or that there is a better way to make the modification, that way we are still able to make an amount of changes that are not considered breaking. It also allows the extension developer to know that they are modifying something that they should expect might change soon and is therefore not considered public API.

The alternative is that more changes to the core frontend are more likely to be breaking changes, which makes maintenance more difficult.

In this case things should remain protected imo. The reason is that doing SignUpModal.isDismissibleViaCloseButton = false; will not work in 2.0

The SignUpModal and most modals are now lazy loaded, so we encourage you to use the extend or override helper instead on a method, and make the modification there, where protected vs public wouldn't matter as it is would be a bound method.

I would suggest keeping the properties readonly, and opening a single point of extension, which would be making the dismissibleOptions instance getter a normal method instead and also public, so that you can do:

override('flarum/forum/components/SignUpModal', 'dismissibleOptions', function () {
  return {
    viaCloseButton: false,
    viaEscKey: this.isDismissibleViaEscKey,
    viaBackdropClick: false,
  };
});

This would both allow customization without creating multiple points of entry 👍🏼

[DavideIadeluca]

Thanks for the detailed insights! Much appreciated.

Closing this one and will reevaluate again once I have a proper 2.x dev environment locally.