-
Notifications
You must be signed in to change notification settings - Fork 26.3k
feat(platform-browser): allow lazy-loading HammerJS #23906
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
Conversation
You can preview 950f46f at https://pr23906-950f46f.ngbuilds.io/. |
This might be a reasonable case for |
@Toxicable the goal is to be agnostic to any particular loading implementation |
I’ve been telling people to stop using Promises because Observables are more powerful. I think it would be good to be consistent in the framework side too |
@@ -58,6 +58,13 @@ const EVENT_NAMES = { | |||
*/ | |||
export const HAMMER_GESTURE_CONFIG = new InjectionToken<HammerGestureConfig>('HammerGestureConfig'); | |||
|
|||
|
|||
/** Function that loads HammerJS, returning a promise that is resolved once HammerJs is loaded. */ | |||
export type HammerLoader = (() => Promise<void>) | null; |
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 | null
correct here ?
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.
I believe so; null
is an acceptable value to provide
for the token
@Inject(HAMMER_GESTURE_CONFIG) private _config: HammerGestureConfig, | ||
private console: Console) { | ||
@Inject(HAMMER_GESTURE_CONFIG) private _config: HammerGestureConfig, private console: Console, | ||
@Optional() @Inject(HAMMER_LOADER) private loader?: HammerLoader) { |
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.
I think | null
should be here instead ?
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.
Per other comment: null
is an acceptable value to provide for the token
export type HammerLoader = (() => Promise<void>) | null; | ||
|
||
/** Injection token used to provide a {@link HammerLoader} to Angular. */ | ||
export const HAMMER_LOADER = new InjectionToken<HammerLoader>('HammerLoader'); |
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.
You need to add public docs and @stable
here
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.
@stable
is no longer needed nor permitted. It is implied from a lack of @deprecated
and @experimental
tags.
@jelbourn Is there documentation somewhere to know how to use this feature? Thanks. |
No documentation yet, but it boils down to providing a function as |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Open for discussion. I made the loader Promise-based instead of Observable for simplicity here, but can change it if folks feel strongly.