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

feat: introduce frontend extenders #3645

Merged
merged 10 commits into from
Jan 17, 2023
Merged

feat: introduce frontend extenders #3645

merged 10 commits into from
Jan 17, 2023

Conversation

SychO9
Copy link
Member

@SychO9 SychO9 commented Sep 16, 2022

Changes proposed in this pull request:
Prior to this, frontend extenders never actually worked before, nor could any extension use them at all as they weren't exported in compat either. This PR does the following:

  • Refactor extenders in core for them to actually work:
    • They are now in the common/extenders directory to avoid conflicts with the extend.ts module).
    • An IExtender interface has been introduced.
    • Th Routes and PostTypes extenders have been adapted to the new changes and the Model extender was removed for a separate PR.
  • Make use of the Routes and PostTypes extenders in bundled extensions.
  • The extenders can be accessed by exposing an extend module from the extension, which contains an array of extenders.

Reviewers should focus on:

  • JS Tests are only failing because check-typings is running before new typings are built.
  • The bundled extension changes were made here as an example, but I can extract them into another PR if wanted. The commits are separated though, so it should be easy to review.

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.

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
@SychO9 SychO9 added this to the 1.6 milestone Sep 16, 2022
@SychO9 SychO9 self-assigned this Sep 16, 2022
@SychO9 SychO9 requested a review from a team as a code owner September 16, 2022 17:36
@SychO9 SychO9 mentioned this pull request Sep 16, 2022
8 tasks
@luceos
Copy link
Member

luceos commented Oct 1, 2022

I don't feel prefixing interfaces with an I is the way to go. We are trying to mimic the ideology of Laravel mostly and Laravel explicitly moved away from ISomething and SomethingInterface to Contracts\Something. Is there a way to do this here as well? For instance naming the IExtender something like Contract\Extends or Contract\Extensible. This, in my experience with Laravel, seemed to be a more eloquent way of using interfaces honestly. What do you think?

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
@SychO9
Copy link
Member Author

SychO9 commented Oct 2, 2022

I don't feel prefixing interfaces with an I is the way to go.

We are already using that convention with other frontend interfaces, so it would be inconsistent tbh.

We are trying to mimic the ideology of Laravel mostly and Laravel explicitly moved away from ISomething and SomethingInterface to Contracts\Something.

That's on the backend side though, and we don't actually mimic Laravel, we explicitly follow the PSR-2 coding style which promotes FooInterface AbstractFoo ...etc

I've changed the class name to ExtenderInterface though

return this;
}

helper(name: string, callback: HelperRoute): Routes {
Copy link
Member

Choose a reason for hiding this comment

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

Is the name "helper" consistent with the concrete implementations included in core's routing helpers? Can we somehow refer to that, even if the type doesn't adds any additional structure or safety, if only for documentation?

Copy link
Member Author

@SychO9 SychO9 Oct 3, 2022

Choose a reason for hiding this comment

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

It's referred to as routeHelpers in core, so I think helper makes sense.

export function makeRouteHelpers(app: ForumApplication) {
return {
/**
* Generate a URL to a discussion.
*/
discussion: (discussion: Discussion, near?: number) => {
return app.route(near && near !== 1 ? 'discussion.near' : 'discussion', {
id: discussion.slug(),
near: near && near !== 1 ? near : undefined,
});
},

framework/core/js/src/common/extenders/Routes.ts Outdated Show resolved Hide resolved
@askvortsov1
Copy link
Member

I don't feel prefixing interfaces with an I is the way to go.

We are already using that convention with other frontend interfaces, so it would be inconsistent tbh.

I would prefer being consistent across the frontend. Different languages have different coding styles, I prefer to stick to convention in each as opposed to forcing an odd middle ground.

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
@SychO9 SychO9 modified the milestones: 1.6, 1.7 Nov 15, 2022
# Conflicts:
#	extensions/mentions/js/src/forum/index.js
#	extensions/package-manager/js/src/admin/index.tsx
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
@SychO9 SychO9 added the javascript Pull requests that update Javascript code label Jan 15, 2023
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.

Great step towards a more consistent, declarative API. Sorry for the review delay!

@SychO9 SychO9 merged commit d7f4975 into main Jan 17, 2023
@SychO9 SychO9 deleted the sm/frontend-extender branch January 17, 2023 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code prio/high type/feature
Projects
Status: completed
Development

Successfully merging this pull request may close these issues.

3 participants