-
-
Notifications
You must be signed in to change notification settings - Fork 834
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
Conversation
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>
I don't feel prefixing interfaces with an |
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
We are already using that convention with other frontend interfaces, so it would be inconsistent tbh.
That's on the backend side though, and we don't actually mimic Laravel, we explicitly follow the PSR-2 coding style which promotes I've changed the class name to |
return this; | ||
} | ||
|
||
helper(name: string, callback: HelperRoute): Routes { |
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.
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?
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.
It's referred to as routeHelpers
in core, so I think helper
makes sense.
framework/framework/core/js/src/forum/routes.ts
Lines 41 to 51 in a595665
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, | |
}); | |
}, |
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>
# Conflicts: # extensions/mentions/js/src/forum/index.js # extensions/package-manager/js/src/admin/index.tsx
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
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.
Great step towards a more consistent, declarative API. Sorry for the review delay!
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:common/extenders
directory to avoid conflicts with theextend.ts
module).IExtender
interface has been introduced.Routes
andPostTypes
extenders have been adapted to the new changes and theModel
extender was removed for a separate PR.Routes
andPostTypes
extenders in bundled extensions.extend
module from the extension, which contains an array of extenders.Reviewers should focus on:
check-typings
is running before new typings are built.Necessity
Confirmed
composer test
).