davidjerleke
commented
7 months ago @donaldxdonald re-opening this because I've updated this PR with all your changes and fixed the corrupted git history here.
Closed donaldxdonald closed 4 months ago
davidjerleke
commented
7 months ago @donaldxdonald re-opening this because I've updated this PR with all your changes and fixed the corrupted git history here.
zip-fa
commented
6 months ago Hi. I was looking for angular wrapper for embla and got here. I saw your code and i have multiple worries about it. 1) You don't init embla outside Angular (ngZone.runOutsideAngular), which can lead to performance issues; also SSR+hydration can break because of setTimeout calls inside mother library our some plugins api 2) You don't use inject() function, but instead you use ctor injection (which is not a best practice for now) 3) You don't use ';'. Which kind of codestyle is this?
I will cherry-pick your code soon and give you link to my repo with everything fixed.
davidjerleke
commented
6 months ago @zip-fa thanks for chiming in.
Before you jump into making changes, please discuss it first and don’t assume you know everything better just because you say so. If something is better in your opinion you should be able to make a case and explain why you think it’s better. Don’t get me wrong, I don’t say that your input or ideas aren’t valuable. They probably are, and I don’t doubt that you mean well. But they just need to be discussed before put into action that’s all.
I can give you an answer for these two things:
also SSR+hydration can break because of setTimeout calls inside mother library our some plugins api
The code is using the function canUseDom() before initializing. Have you seen this? It prevents the Embla script from running at all on the server.
You don't use ';'. Which kind of codestyle is this?
This project uses prettier which is an automatic code formatter and it’s configured to not use semi colons. Even though prettier has a little bit of configurable options, they are few and it’s meant to put an end to religious discussions about code formatting so this is not up for discussion to change.
I’m going to let @donaldxdonald answer about point 2 because I don’t know much about how Angular works.
Best, David
zip-fa
commented
6 months ago https://github.com/zip-fa/ng-embla-carousel
here you go. We can discuss everything through twitter, maybe it would be better? What did i do:
1) Do not use NgModule (because everything is standalone in 2023) 2) Use afterNextRender method (canUseDom() is redundant; afterNextRender WILL NOT be executed inside SSR) 3) ngZone.runOutsideAngular will ensure ChangeDetector to not trigger because of Embla's apis 4) Made your DI token optional (because why we need that much code with default factory? just don't provide token and that's all)
I saw unused @Output(), will you use it?
zip-fa
commented
6 months ago Take a look here (still dirty, but main idea shown):
@Input({ required: true })
set ngEmblaCarousel(options: EmblaOptionsType) {
this.options = options;
this.reInit();
}
@Input({ alias: 'plugins' })
set _plugins(plugins: EmblaPluginType[]) {
this.plugins = plugins;
this.reInit();
}ngOnChanges usage is redundant. Just use setters, they will be executed each time new value comes. ngEmblaCarousel is required input, so i made it { required: true }, which enforce user to setup embla (if i got an idea correctly; we can discuss it and remove required option if i am wrong)
Also you have this code in your initial version: emblaApi!: EmblaCarouselType
But emblaApi can be undefined inside ssr environment.
Sorry if i don't understand something, that's because i found embla-carousel two hours ago :)
zip-fa
commented
6 months ago Sorry for spamming, but one more thing.
Angular has new apis from v16:
destroy() callback only when i 100% sure embla is initialized (in browser environment)There is a lot to do with my implementation, but it's much cleaner in terms of logic (codestyle can be changed to match your repo's style).
davidjerleke
commented
6 months ago @zip-fa thank you for your input. If you want to demonstrate your ideas with your code and how you would do it, in order to make them more clear and avoid confusion, you can create a CodeSandbox like @donaldxdonald did here before he created this pull request.
zip-fa
commented
6 months ago davidjerleke
commented
6 months ago @zip-fa a CodeSandbox can be tested and forked + edited in matter of a second compared to a GitHub repository that needs to be cloned, setup and it’s not as easy and fast to make iterations and share ideas. Until all agree upon the final product/solution it’s much faster to work with.
zip-fa
commented
6 months ago Got it. I’ll polish everything first and share you a minimal setup with codesandbox. How about switching our conversation from github to twitter? Mine is: twitter.com/buzahuza
davidjerleke
commented
6 months ago @zip-fa I would recommend to keep the conversation here either in a new discussion or in the original issue:
And the reason for this is because it’s better to have everything in one place so devs can see what discussions led to the Angular wrapper becoming reality. And GitHub is the place for code and code related conversations (specifically this repo is relevant) whereas Twitter/X is a place for everything you can think of.
I would also encourage you to read the conversation between me and @donaldxdonald in the original issue to understand what my vision with Embla is and how the other existing framework wrappers work (like react and vue):
zip-fa
commented
6 months ago Hi again.
I polished everything & added stackblitz example with everything working: https://stackblitz-starters-4buajt.stackblitz.io
Github example updated too (library is buildable & demo working): https://github.com/zip-fa/ng-embla-carousel
What have been done:
Insted, i use afterNextRender hook:
constructor() {
afterNextRender(
() => this.init(),
{ phase: AfterRenderPhase.Write }
);
}It gets called only inside browser environment, so it's safe to manipulate DOM with it. Link to angular docs
inject() function instead of ctor DIThis is the new standard from v14 (which is released few years ago):
private readonly elementRef = inject(ElementRef);
private readonly globalOptions = inject(EMBLA_OPTIONS_TOKEN);
private readonly ngZone = inject(NgZone);
private readonly destroyRef = inject(DestroyRef);There is more cleaner approach: DestroyRef.onDestroy(), which will get called on template destruction.
because why should i? standalone apis came to angular from v14 (which is deprecated already, and released few years ago)
scrollTo(index: number, jump?: boolean): void {
this.ngZone.runOutsideAngular(() => this.emblaApi?.scrollTo(index, jump));
}
scrollPrev(jump?: boolean): void {
this.ngZone.runOutsideAngular(() => this.emblaApi?.scrollPrev(jump));
}
scrollNext(jump?: boolean): void {
this.ngZone.runOutsideAngular(() => this.emblaApi?.scrollNext(jump));
}
private init(): void {
if (this.globalOptions) {
EmblaCarousel.globalOptions = this.globalOptions;
}
this.ngZone.runOutsideAngular(() => {
this.emblaApi = EmblaCarousel(
this.elementRef.nativeElement,
this.options,
this.plugins
);
this.listenEvents();
this.destroyRef.onDestroy(() => this.destroy());
});
}
private reInit(): void {
if (!this.emblaApi) {
return;
}
this.ngZone.runOutsideAngular(() => {
this.emblaApi!.reInit(this.options, this.plugins);
});
}Because of how Angular works, we have to call ngZone.runOutsideAngular to not trigger ChangeDetection mechanism.
zone.js monkey-patches every event: setTimeout, setInterval, etc..., and when setInterval gets called, Angular's ChangeDetector triggers and app performance degrades.
https://github.com/davidjerleke/embla-carousel/assets/61551308/3b8df028-eb5c-426d-9da4-75bed3f9b0e8
https://github.com/davidjerleke/embla-carousel/assets/61551308/020f6e2e-9c38-40ef-8e58-497e41209741
... but with simpler logic. Since the main task is to call reInit() method each time plugins or options @Input() changes, i refactored ngOnChanges:
@Input({ alias: 'emblaCarousel' })
public options: EmblaOptionsType = {};
@Input({ alias: 'emblaPlugins' })
public plugins: EmblaPluginType[] = [];
ngOnChanges(changes: SimpleChanges) {
const optionsChange = changes['options'] as SimpleChange | undefined;
const pluginsChange = changes['plugins'] as SimpleChange | undefined;
if(
(optionsChange && !optionsChange.firstChange) ||
(pluginsChange && !pluginsChange.firstChange)
) {
this.reInit();
}
}Removed redundant helpers to compare objects, i just look at firstChange - that's all.
Now, my directive reacts to every Embla event:
'init', 'pointerDown', 'pointerUp', 'slidesChanged', 'slidesInView', 'select', 'settle', 'destroy', 'reInit', 'resize'
By default, scroll is not included here, because it fires too often. It can be added with provideEmblaOptions in app.config.ts:
providers: [
provideEmblaOptions({
eventsThrottleTime: 100, // <-- configure throttle to avoid performance bottlenecks
listenEvents: [
'init',
'pointerDown',
'pointerUp',
'slidesChanged',
'slidesInView',
'select',
'settle',
'destroy',
'reInit',
'resize',
'scroll' // <-- manually add scroll here
]
})
]As you can see, there is eventsThrottleTime also. I added throttler based on rxjs in my code, to fix scroll event, which fires too frequently:
private listenEvents(): void {
const { eventsThrottleTime, listenEvents } = this.globalOptions;
// This code is valid by the api design: `provideEmblaOptions({ eventsThrottleTime: undefined, listenEvents: undefined })`
if (!eventsThrottleTime || !listenEvents) {
return;
}
const eventsThrottler$ = new Subject<EmblaEventType>();
eventsThrottler$
.pipe(
throttleTime(eventsThrottleTime),
takeUntilDestroyed(this.destroyRef)
)
.subscribe((eventName) => {
this.ngZone.run(() => this.emblaChange.emit(eventName));
});
this.ngZone.runOutsideAngular(() => {
listenEvents.forEach((eventName) => {
this.emblaApi!.on(eventName, () => eventsThrottler$.next(eventName));
});
});
}@zip-fa thanks for your efforts.
Removed redundant helpers to compare objects, i just look at firstChange - that's all.
How exactly does this work under the hood? When is it considered a change? When the reference change?
I implemented (emblaChange) event
Now, my directive reacts to every Embla event
I think you have missed the Embla vision. Please, no event and feature wrappers. Just expose the raw Embla API for maximum flexibility. Please read this comment in detail.
zip-fa
commented
6 months ago How exactly does this work under the hood? When is it considered a change? When the reference change?
Angular compares two objects (old and new), and then ngOnChanges triggers
Please, no event and feature wrappers
That's not how angular 3rd-party libs must work. By not wrapping embla's events and scrollTo/scrollPrev/scrollNext, you will break app performance. I attached two videos on how change detection works in Angular. Please take a look at them, you will notice how many times ChangeDetector is triggered. There is no other way for now, because zone.js is still a part of Angular. Angular's team will make it optional soon, but for now, if not wrap these three methods and every event - RIP for performance.
Also, i published my wrapper to npmjs for those who want to try
Just expose the raw Embla API for maximum flexibility
it is exposed:
@ViewChild(EmblaCarouselDirective)
private readonly emblaCarousel!: EmblaCarouselDirective;
someFunc(): void {
this.emblaCarousel.emblaApi // here it goes
}You can still access every embla api method, but it will lead to performance issues with manual subscription to events:
this.emblaCarousel.emblaApi.on('scroll', () => console.log('i will kill your app'));
this.emblaCarousel.emblaApi.scrollPrev(); // i will kill your app too
this.emblaCarousel.emblaApi.scrollNext(); // me too!Preferred way to call scrollPrev/scrollNext:
this.emblaCarousel.scrollPrev(); // i am performant
this.emblaCarousel.scrollNext(); // i am performant too!Listen to every event in component.html:
<div class="embla__viewport"
[emblaCarousel]="{ loop: true }"
(emblaChange)="onEmblaChanged($event)"
>
<div class="embla__container">
@for (slide of slideImages(); track $index) {
<div class="embla__slide">
<div class="embla__slide__number"><span>{{ $index + 1 }}</span></div>
<img class="embla__slide__img" [src]="slide" alt="Your alt text">
</div>
}
</div>
</div>component.ts:
onEmblaChanged(event: EmblaEventType): void {
console.log('no performance bottlenecks: ', event);
}Angular compares two objects (old and new), and then ngOnChanges triggers
So this is a case where I start to doubt your judgement a bit because you say:
Removed redundant helpers to compare objects, i just look at firstChange - that's all.
How do you know they’re redundant? I don’t know if you read the discussion I encouraged you to do but me and @donaldxdonald already discussed this. The utilities are there to prevent the carousel from re-initializing unless options and plugins actually change. It doesn’t care about reference changes but it compares the options and plugin object contents.
You can still access every embla api method, but it will lead to performance issues with manual subscription to events.
Are we talking about micro optimization and performance issues that no one will almost never notice or real problems? It’s hard for me to believe that Angular doesn’t have any way of exposing an API without destroying the browser? Unless there’s proof of a clear performance hit, readability and vision always have the highest priority.
One problem with building another layer of API wrapping is that it needs to be maintained. Every time an event changes or gets added/removed it needs a code change. In contrast, the React, Vue and Svelte wrappers don’t need any manual labor to update as soon as the Embla API changes which is much better in the long run. Again, me and @donaldxdonald have already had this conversation so if you haven’t, please read the conversation here:
Best, David
zip-fa
commented
6 months ago That's not a micro-optimization. Your setInterval() code will trigger angular to re-check every template each time it executes (every 10ms in your case, check my videos please).
Yes, i've read your conversation, but as i said, your implementation will kill an app without ngZone.runOutsideAngular().
About ngOnChanges: i don't really understand why you want to put this double-check. Angular will not trigger ngOnChanges unless @Input() is really changed. Please add console.log() inside ngOnChanges on stackblitz and try to play with its values.
Embla APIs are still exposed, because they are public in @Directive() and can be accessed by developers.
I gave you two videos with Angular extension installed, where you can clearly see how it's ChangeDetector triggers, you ask me to prove what? Please do some research about this topic: performance bottlenecks and runOutsideAngular to better understand how Angular's change detection mechanism works. Sorry if it looks a bit rude, but i don't really understand what i have to prove if you didn't even watch my videos.
Best, zip-fa.
donaldxdonald
commented
6 months ago Hi @zip-fa ,
Sorry for my delayed response. I have been quite occupied recently and was unable to engage in the conversation in time. It's great to see more people contributing to the Angular wrapper!
I also agree with @davidjerleke that discussing it in one place (here) allows more developers who come here to know as much context as possible. These are my thoughts.
Yes, you're right, I realised later that I had to add this as well. But David said not to push new stuff until he's finished reviewing it. I just thought I'd update the code in a later version.
inject function vs constructor DII like to use the inject function as well. The first version of my example was based on Angular 13, which didn't have the inject function yet.
When I changed it to Angular 14, I didn't change it, and in my case, the difference is not a big problem. In my case, the difference is not a big problem. And neither the angular.io nor the angular.dev official Angular docs say that constructor DI is bad.
The version I've provided doesn't force the use of ngModule, but instead provides both ngModule and standalone ways to use the library.
Considering that there are still a lot of Angular developers who prefer to use ngModule, as a library developer you have to consider the needs of a wider range of users.
emblaChange OutputYes, I forgot to delete it when updating up from the old code, and as David said, try to use a style that aligns with the version of the other framework.
The reason to use the "redundant" helper comes from here, the second one.
ngOnChanges detects changes to object values, but not to object property references. For example, a change in options as a whole will trigger change detection, but a change in options.active will not.
In fact, I think encapsulating options into more granular props would be more convenient because often we need to dynamically update a specific option.
afterNextRender, DestroyRef and other new APIsThe new version of Angular is great. But as I said above, as library developers we should be thinking about covering more users, and as embla-carousel's first version of the Angular wrapper, I think support for covering more Angular versions is a more important thing. I chose to support Angular 14 as a minimum because the standalone component/directive starts here. Support for more new APIs could be added in perhaps a second release.
Sorry I haven't used the new @angular/ssr. I've just used Angular Universal before and had a terrible experience. Maybe support can be added later.
Finally, I'd like to say that this version of mine started out as a way to share what I was using at work, since no one had contributed Angular wrapper code for a long time before. Then I worked with David to refine it.
I don't always have the time to maintain a follow-up to this code, so I chose to put the code in the main embla-carousel repository instead of my own. This allows more developers to keep improving it. So I will respect David's need for a consistent style. Of course, it's also possible to release another repository like you did separately.
I think technical communication is a great thing, and I welcome working together to improve the code.
Best regards
zip-fa
commented
6 months ago Hi @donaldxdonald. Glad you joined that talk.
For me, as a developer, i don't want to wrap everything by myself with ngZone.runOutsideAngular. Library MUST support it out-of-the box, i don't want to really care about performance and make a repeated code.
It's not maintained anymore. Also, Angular 15 support will be dropped soon: https://angular.dev/reference/releases#release-schedule
Since afterRender and DestroyRef were introduced in v16, i guess sticking to V16 will be much better
I don't really understand why we need to give developers "a choice" to use outdated pattern. You can use standalone APIs inside NgModules, but giving users a choice to use NgModules in standalone APIs is a bad idea.
Recently, Angular dropped support of mutable changes inside signal() apis. And i think, mutating reactive data is a bad idea. Making CPU-consuming comparator functions just to support it.. seems like an anti-pattern.
That would be a bad idea. Because that realisation must be maintained :) Please don't do this
Since v17, angular's ssr experience is great. I've "added support" for it just with afterRenderer (embla will not be initialized on the server, while slides will be rendered).
davidjerleke
commented
6 months ago Hi again @zip-fa,
That's not a micro-optimization. Your
setInterval()code will trigger angular to re-check every template each time it executes (every 10ms in your case, check my videos please). Yes, i've read your conversation, but as i said, your implementation will kill an app without ngZone.runOutsideAngular().
I haven't said anything about using ngZone.runOutsideAngular() is a bad idea or wrong anywhere in my comments? I have no objections here and trust that you know this. I certainly don't know Angular and how it works.
My objection was that wrapping the Embla API in another layer requires more maintenance because it has to be updated as soon as any Embla event changes or gets added/removed.
About the setInterval() you mention that runs every 10ms. What interval are you talking about? Do you mean the init event wrapped in a setTimeout? If yes, the init event is deprecated, will be removed and is the only event that uses a timeout. Of course, you couldn't have known that it's deprecated so that's my bad. But it is and won't be a problem anymore. If no, please point me to the correct piece of code you're talking about so I can understand you better.
About ngOnChanges: i don't really understand why you want to put this double-check. Angular will not trigger ngOnChanges unless @Input() is really changed. Please add console.log() inside ngOnChanges on stackblitz and try to play with its values.
A double-check means that you check the exact same thing twice. This is certainly not the case with the areOptionsEqual and arePluginsEqual helpers. JavaScript objects are compared and stored by reference, not by value. An example:
const optionsA = { loop: true }
const optionsB = { loop: true }
console.log(optionsA === optionsB) // --> falseIn the example above, the contents of the both option objects are identical but still, they are not considered equal, and a re-assignment of the options would trigger the ngOnChanges hook. In this case, I would argue that Embla shouldn't re-initialize because nothing has actually changed. The areOptionsEqual and arePluginsEqual helpers prevent Embla from re-initializing in these cases. And while my example is trivial and might seem to not happen in reality, it does. That's why it was introduced in the first place.
Embla APIs are still exposed, because they are
publicin @Directive() and can be accessed by developers.I gave you two videos with Angular extension installed, where you can clearly see how it's ChangeDetector triggers, you ask me to prove what? Please do some research about this topic: performance bottlenecks and runOutsideAngular to better understand how Angular's change detection mechanism works. Sorry if it looks a bit rude, but i don't really understand what i have to prove if you didn't even watch my videos.
First off, I have watched your screen recordings and do appreciate the effort. And I don't think it makes sense to tell me to do the Angular research because if I knew and studied Angular I would have built the wrapper myself. I trust your knowledge and want to be constructive because I know how Embla works under the hood and I have a vision with Embla.
When we work as devs, it's not like customers comes to us and say "hey, build anything you want! We have no requirements!". Naturally, they have requirements about what kind of app/website they need and what features it should include. Similarly, me as the Embla creator and maintainer have a vision with Embla and I will have some requirements if you want to help out and create a library/framework wrapper. In this specific case an official Angular wrapper. However, given the requirements, I don't want to dive in to details because I trust you to solve that as long as you respect the requirements. In some cases we might have to compromise because of framework limitations or similar and I understand that. You can see that this has been the case when me and @donaldxdonald had our conversation.
I've been part of building the react, vue and svelte wrappers and they all expose the raw API, have no problems with re-rendering the whole app all the time. They only do the minimum amount of things to give devs a seamless integration with their frameworks. And the best of all, they require no maintenance when the Embla API changes - because they expose the raw API. All of the above mentioned frameworks can do that. If you're telling me that Angular really can't expose an API without destroying the entire webpage and mean it, well maybe that's the case then.
I want to be constructive and don't want to control all details, I trust you guys on that and your competence. But please take a moment to read the requirements for an official library wrapper. There's no point for me in creating just another carousel library that works just as all others out there. Embla aims to solve the hardest technical challenges with building carousels, and the rest is up to the user utilizing its highly extensible API and plugin system.
Now I understand that there might be one or more requirements that aren't doable because of Angular limitations or another reason. If that's the case, all I'm asking for is that you take the time to explain why to me and what the alternatives are because I know nothing about Angular:
All library wrappers are minimal wrappers that just wrap the core embla-carousel package for seamless integration and what they do is basically the following:
options and plugins as parameters.emblaApi.reInit() if options or plugins change (deep equality, not reference).ref for users to freely attach to the DOM themselves. globalOptions feature.emblaApi for maximum flexibility.I will take the embla-carousel-react example to explain:
import { useRef, useEffect, useState, useCallback } from 'react'
// Feel free to use this in your Angular wrapper 👇
import {
areOptionsEqual,
arePluginsEqual,
canUseDOM
} from 'embla-carousel-reactive-utils'
import EmblaCarousel, {
EmblaCarouselType,
EmblaOptionsType,
EmblaPluginType
} from 'embla-carousel'
type EmblaViewportRefType = <ViewportElement extends HTMLElement>(
instance: ViewportElement | null
) => void
export type UseEmblaCarouselType = [
EmblaViewportRefType,
EmblaCarouselType | undefined
]
function useEmblaCarousel(
options: EmblaOptionsType = {},
plugins: EmblaPluginType[] = []
): UseEmblaCarouselType {
const storedOptions = useRef(options)
const storedPlugins = useRef(plugins)
const [emblaApi, setEmblaApi] = useState<EmblaCarouselType>()
const [viewport, setViewport] = useState<HTMLElement>()
const reInit = useCallback(() => {
if (emblaApi) emblaApi.reInit(storedOptions.current, storedPlugins.current)
}, [emblaApi])
useEffect(() => {
// IF REF IS ATTACHED TO HTML - Initialise Embla and store the emblaApi which will be exposed
if (canUseDOM() && viewport) {
// GLOBAL OPTIONS - Make use of global options and pass it to the core library
EmblaCarousel.globalOptions = useEmblaCarousel.globalOptions
const newEmblaApi = EmblaCarousel(
viewport,
storedOptions.current,
storedPlugins.current
)
setEmblaApi(newEmblaApi)
return () => newEmblaApi.destroy()
} else {
// IF REF IS NOT ATTACHED TO HTML - Set emblaApi to undefined
setEmblaApi(undefined)
}
}, [viewport, setEmblaApi])
// IF OPTIONS CHANGE - Run emblaApi.reInit()
useEffect(() => {
// use areOptionsEqual() from the utils package to compare
if (areOptionsEqual(storedOptions.current, options)) return
storedOptions.current = options
reInit()
}, [options, reInit])
// IF PLUGINS CHANGE - Run emblaApi.reInit()
useEffect(() => {
// use arePluginsEqual() from the utils package to compare
if (arePluginsEqual(storedPlugins.current, plugins)) return
storedPlugins.current = plugins
reInit()
}, [plugins, reInit])
// EXPOSE - 0: A ref to attach to the DOM element that will be the carousel
// EXPOSE - 1: The raw emblaApi
return [<EmblaViewportRefType>setViewport, emblaApi]
}
useEmblaCarousel.globalOptions = <EmblaOptionsType | undefined>undefined
export default useEmblaCarouselimport React, { useEffect } from 'react'
import useEmblaCarousel from 'embla-carousel-react'
import Autoplay from 'embla-carousel-autoplay'
export const EmblaCarousel = () => {
// Usage of the useEmblaCarousel() wrapper
const [emblaRef, emblaApi] = useEmblaCarousel({ loop: false }, [Autoplay()])
useEffect(() => {
if (emblaApi) {
console.log(emblaApi.slideNodes()) // Access API
}
}, [emblaApi])
return (
<div className="embla" ref={emblaRef}>
<div className="embla__container">
<div className="embla__slide">Slide 1</div>
<div className="embla__slide">Slide 2</div>
<div className="embla__slide">Slide 3</div>
</div>
</div>
)
}Best, David
Originally posted by @davidjerleke in https://github.com/davidjerleke/embla-carousel/issues/18#issuecomment-1670979561
Best, David
davidjerleke
commented
6 months ago And @donaldxdonald sorry about "freezing" this PR. You are all free to make new commits and continue working on this now that we have more devs like @zip-fa and @JeanMeche helping out. Let me know when you all are satisfied with the wrapper and I will just review basic things that are related to the repository setup and alike.
Best, David
zip-fa
commented
6 months ago Since my holidays ended and i have to do my usual work, i will return to this PR and response at the end of this week. However, i just published README for my implementation, you can check it out: https://github.com/zip-fa/ng-embla-carousel and cherry-pick whatever you want.
Thanks, will reach you asap!
donaldxdonald
commented
5 months ago The new @angular/ssr was introduced in Angular v17, and embla-carousel-angular will require a minimum installation of Angular v17 if it is to be supported immediately.
The current embla-carousel-angular will support Angular 14+ and will work fine in Angular v17 projects (non-SSR) like embla-carousel-playground-angular.
I think that's good enough.
davidjerleke
commented
5 months ago @donaldxdonald so you guys are done then? Or do you want to add something more?
Best, David
donaldxdonald
commented
5 months ago I have finished my part, and I'm unsure if @zip-fa and @JeanMeche have any additional comments or suggestions.
davidjerleke
commented
5 months ago @zip-fa, @JeanMeche do you want to add something?
zip-fa
commented
5 months ago Hi! Sorry for not replying, was busy. Gimme some time, i will check everything out!
zip-fa
commented
5 months ago Everything looks ok, but... there are two things that i am worried about:
1) Events listening
2) Raw embla api access for these methods: scrollTo, scrollPrev, scrollNext
In this realisation, dev MUST write this code to simply subscribe to any of embla's event:
template.html:
<div
ngx-embla
>
...
</div>template.ts:
export class Component implements AfterViewInit {
private ngZone = inject(NgZone);
@ViewChild(EmblaDirective)
private emblaDirective?: EmblaDirective;
ngAfterViewInit(): void {
// what if in this stage directive is not initiailized because of @if() logic ?
// then dev must write setter for @ViewChild(EmblaDirective) to access it when it become available
this.ngZone.runOutsideAngular(() => {
this.emblaDirective?.emblaApi?.on('someEvent', () => {
this.ngZone.run(() => this.doSomeLogic());
});
});
}
private doSomeLogic(): void {
// application logic
}
}This looks like overkill to do that simple thing. I know you want to make it less time-consuming to maintain, but DX is better with these few lines of code inside directive: click here
In my realisation, code will become much simplier in terms of DX:
template.html:
<div
ngx-embla
[listenEvents]="['someEvent']"
(emblaChange)="doSomeLogic()"
>
...
</div>template.ts:
doSomeLogic(): void {
}which wraps .on() method with ngZone.runOutsideAngular()
scrollTo, scrollPrev, scrollNextThese three methods calls depends on Animations.ts.
requestAnimationFrame() call will trigger ngZone to mark all app as dirty.
This this call is recursive:
(animate -> requestAnimationFrame(animate) -> animate -> requestAnimationFrame(animate)), and it triggers too much change detection, which kills app performance with this simple code (current realisation):
template.html:
<div
ngx-embla
#emblaRef="emblaNode"
>
...
</div>
<button (click)="emblaRef?.emblaApi.scrollPrev()">prev slide</button>
<button (click)="emblaRef?.emblaApi.scrollNext()">next slide</button>You can check my video again, where i show how much times angular re-runs change-detection cycle for the WHOLE APP. I strongly recommend to wrap these three calls with runOutsideAngular() inside directive:
davidjerleke
commented
5 months ago @zip-fa I’m sorry if I wasn’t clear:
You are allowed to wrap the API with another layer if you want and if you think that’s the solution that makes sense. I just wanted to share the Embla vision and what I think the aim should be for this wrapper (see requirements in this comment. The requirements should act as guidelines when building this wrapper.
My main concern with another layer of API wrapping is the maintenance that it requires so we should only do that when we don’t see another solution. But if that’s the only solution that makes sense, go with it!
Beat, David
zip-fa
commented
5 months ago You are 100% right about maintenance cost of additional layer of wrappers
Angular is going to make zone.js optional, but in the next few years it will be part of Angular, so we need to make a wrapper. Sorry to say that, but that's the only solution here...
davidjerleke
commented
5 months ago You are 100% right about maintenance cost of additional layer of wrappers
Angular is going to make zone.js optional, but in the next few years it will be part of Angular, so we need to make a wrapper. Sorry to say that, but that's the only solution here...
@zip-fa I understand. Then go with it 👍🏻!
Still, the raw API will still be exposed too right? To give users maximum flexibility?
zip-fa
commented
5 months ago Yeah, raw api is still exposed. You can play with it how you want. I made full documentation with examples on HOW IT WILL LOOK LIKE in official wrapper here:
https://github.com/zip-fa/ng-embla-carousel/tree/dev (check last paragraph - Access embla api)
What do we do with event listeners? Do you accept my solution?
If we agree on event listeners mechanism and on scrollTo, scrollPrev, scrollNext wrapper, then @donaldxdonald can safely cherry-pick this from my code:
@Input()
public subscribeToEvents: EmblaEventType[] = [];
@Input()
public eventsThrottleTime = 100;
@Output()
public readonly emblaChange = new EventEmitter<EmblaEventType>();
private init(): void {
// logic here
this.listenEvents():
}
/**
* `eventsThrottler$` Subject was made just because `scroll` event fires too often.
*/
private listenEvents(): void {
if (0 === this.subscribeToEvents.length) {
return;
}
const eventsThrottler$ = new Subject<EmblaEventType>();
eventsThrottler$
.pipe(
throttleTime(this.eventsThrottleTime),
takeUntilDestroyed(this.destroyRef)
)
.subscribe((eventName) => {
this.ngZone.run(() => this.emblaChange.emit(eventName));
});
this.ngZone.runOutsideAngular(() => {
this.subscribeToEvents.forEach((eventName) => {
this.emblaApi!.on(eventName, () => eventsThrottler$.next(eventName));
});
});
}@zip-fa thanks for the details. I’m going to let @donaldxdonald and @JeanMeche answer your questions here because I don’t know Angular. Let me know when you’re satisfied with the state of this PR and I will just skim through repository related stuff.
I’m always here if you have any questions related to the core library.
Thank you all for your time and efforts!
Best, David
donaldxdonald
commented
5 months ago emblaCarouselscrollTo, scrollPrev, scrollNext methodsemblaChange eventembla-carousel-playground-angularAccessing the carousel API and Listening the carousel events in docs/get-started/angular.scrollTo, scrollPrev, scrollNext methodsTo reduce the maintenance cost, the parameters directly use the parameters of the embla API.
@Directive({})
export class EmblaCarouselDirective {
scrollTo(...args: Parameters<EmblaCarouselType['scrollTo']>): void {
this.ngZone.runOutsideAngular(() => this.emblaApi?.scrollTo(...args))
}
scrollPrev(...args: Parameters<EmblaCarouselType['scrollPrev']>): void {
this.ngZone.runOutsideAngular(() => this.emblaApi?.scrollPrev(...args))
}
scrollNext(...args: Parameters<EmblaCarouselType['scrollNext']>): void {
this.ngZone.runOutsideAngular(() => this.emblaApi?.scrollNext(...args))
}
}cc @zip-fa, @JeanMeche
zip-fa
commented
5 months ago zip-fa
commented
5 months ago any chances to get inside contributors list?:))
donaldxdonald
commented
5 months ago any chances to get inside contributors list?:))
I agree, but it's up to @davidjerleke .
davidjerleke
commented
5 months ago @donaldxdonald, @zip-fa, @JeanMeche you will all be added to the special thanks section in all package README’s, because you’ve made this happen 👍🏻.
@donaldxdonald you will also be added to the regular contributors list, because you created this PR and that list is automatically generated with the build process.
I can add all of you as co-authors when I squash all commits here before merging, but don’t know if that will add all of you as contributors here? Not sure if it works that way.
Best, David
zip-fa
commented
5 months ago Special thanks section is already very pleasant for me. Thanks!
donaldxdonald
commented
5 months ago Hi guys @donaldxdonald, @zip-fa, and @JeanMeche,
So I finally got some time off to look into this PR. Great work ⭐!
In addition to the comments I left I have one concern: It doesn't seem like the Angular bundler creates support for
moduleResolution: nodeNext? Or maybe I did something wrong? Read more about it here:Because all
emblapackages are hybrids and does support that. It's automatically added upon the package build process for each package. I'm just thinking that this problem will just get bigger soon as more devs start using it, and if the Angular build tool isn't doing extraordinary stuff it might be worth using the embla package bundler instead?I look forward to your feedback.
Best, David
Hi, @davidjerleke ,
As mentioned earlier, Angular has its own build tool called the Angular CLI, which is based on Webpack (and esbuild starting from Angular v17+). With the Angular CLI, Angular developers don't have to worry about low-level bundling configurations, and the output is compatible with projects using the same or higher Angular versions. For example, embla-carousel-angular is built using Angular CLI v14, which means it can be used in almost all Angular 14+ projects without any issues.
Since its early days, the Angular CLI has been generating bundles only in the ECMAScript Modules (ESM) format (more info), and the Angular team does not recommend using CommonJS format libraries in Angular projects. Reference
It is recommended that you avoid depending on CommonJS modules in your Angular applications. Depending on CommonJS modules can prevent bundlers and minifiers from optimizing your application, which results in larger bundle sizes. Instead, it is recommended that you use ECMAScript modules in your entire application.
Compared to other UI frameworks, Angular is more like a true "framework," and around 99% of Angular projects/libraries use the Angular CLI for bundling. Unlike other frameworks, there are not many variations in "best practices." Therefore, as an Angular library, using the Angular CLI to build embla-carousel-angular is the most suitable approach.
davidjerleke
commented
4 months ago @donaldxdonald thanks ⭐! Sorry for the late response. Me and my family got the flu.
About this wrapper - I've been having this nagging feeling that I don't understand how Angular works and therefore I don't understand you guys when you explain things to me. So in order to understand you guys better, I took this Angular course.
I just want to say that I'm sorry if I limited your plans for this Angular wrapper. I know understand that Angular is completely different from React, Vue, Svelte and Solid - Libraries of which I've been part of creating Embla wrappers for. This also made me realise that it makes sense that the Angular wrapper for Embla can be designed very differently from the other wrappers - Whether it should completelty wrap the whole Embla API, its methods and events or not, that's up to you and I won't interfere anything from now on.
I'm working on shared build tools and automating a lot of stuff in this repository for all the other wrappers - Which is doable because they're all very similar in how they work in contrast to Angular. So because Angular is so different in its nature, it might make sense that you @donaldxdonald, create a separate repository for the embla-carousel-angular wrapper? I see many benefits with this approach:
What do you think?
Best, David
donaldxdonald
commented
4 months ago Hi @davidjerleke ,
I'm sorry for being busy lately and not replying promptly. As you suggested, creating a separate repository is definitely doable. I'll find some time in the next few days to organize the code. Take care : )
donaldxdonald
commented
4 months ago Hi @davidjerleke ,
The initial version of embla-carousel-angular has been released, and I will continue to maintain it. For the documentation I think it would be good to maintain it in one place, in the embla-carousel-angular repository. Thanks for the suggestion.
Repo: https://github.com/donaldxdonald/embla-carousel-angular NPM: https://www.npmjs.com/package/embla-carousel-angular
davidjerleke
commented
4 months ago @donaldxdonald that was fast. Nice work, I think it looks great 🚀! You just got your first ⭐.
For the documentation I think it would be good to maintain it in one place, in the
embla-carousel-angularrepository.
Makes sense. Let me know if you change your mind.
Is it ok for you if I add the Angular logo and link it to your repository here?
I think it would be good for devs to know that there's an official Angular wrapper out there 🙂.
Best, David
donaldxdonald
commented
4 months ago Is it ok for you if I add the Angular logo and link it to your repository here?
I think it's a great idea. Thanks @davidjerleke !
davidjerleke
commented
4 months ago Thanks for all your efforts here @donaldxdonald, @zip-fa and @JeanMeche!
davidjerleke
commented
4 months ago @donaldxdonald I think this is cool: The first two users of embla-carousel-angular 🎉!
I've added the link to the Angular wrapper now:
If you have a Reddit account you can share the news that embla-carousel-angular is out in an Angular group or similar. That could increase traction. Nice work ✨!
Hi @davidjerleke ,
Long time no talk! I apologize for the delay, as I've been quite busy lately. I wanted to let you know that I've finished all the Angular wrapper code and documentation. It's all ready for review. The context is here #18 .
There are a few points that need to be clarified:
Bundling
Angular has its own webpack-based Angular CLI for bundling, and since the Angular ecosystem is almost exclusively packaged using the Angular CLI, it makes more sense to use it for bundling.
Publishing
As mentioned above,
embla-carousel-angularis packaged using the Angular CLI, which outputs the build to thedistfolder (or some other non-root path).All other libraries in the
embla-carouselrepository are packaged with rollup and exported directly to the root directory of the library, and then thenpm publishcommand is executed in CI/CD, which is not directly usable inembla-carousel-angular.So I've changed the release command in
cd.ymlto runyarn releaseso that I can use a specific release strategy for each library.Thank you for your patience, and once again, I apologize for the delay. I appreciate your continued support and guidance throughout this process.