-
-
Notifications
You must be signed in to change notification settings - Fork 196
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]: Angular wrapper #627
[Feat]: Angular wrapper #627
Conversation
a7e87ea
to
d1d220f
Compare
@donaldxdonald re-opening this because I've updated this PR with all your changes and fixed the corrupted git history here. |
2023cec
to
7259e79
Compare
Hi. I was looking for angular wrapper for embla and got here. I saw your code and i have multiple worries about it.
I will cherry-pick your code soon and give you link to my repo with everything fixed. |
@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:
The code is using the function
This project uses I’m going to let @donaldxdonald answer about point 2 because I don’t know much about how Angular works. Best, |
https://github.com/zip-fa/ng-embla-carousel here you go.
I saw unused @output(), will you use it? |
Take a look here (still dirty, but main idea shown):
ngOnChanges usage is redundant. Just use setters, they will be executed each time new value comes. Also you have this code in your initial version: 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 :) |
Sorry for spamming, but one more thing. Angular has new apis from v16:
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). |
@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 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. |
Got it. I’ll polish everything first and share you a minimal setup with codesandbox. |
@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): |
Hi again. I polished everything & added stackblitz example with everything working: Github example updated too (library is buildable & demo working): What have been done: afterViewInit & hasDom() methods removedInsted, i use
It gets called only inside browser environment, so it's safe to manipulate DOM with it. Link to angular docs I use
|
@zip-fa thanks for your efforts.
How exactly does this work under the hood? When is it considered a change? When the reference change?
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. |
Angular compares two objects (old and new), and then ngOnChanges triggers
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
it is exposed:
You can still access every embla api method, but it will lead to performance issues with manual subscription to events:
Preferred way to call scrollPrev/scrollNext:
Listen to every event in component.html:
component.ts:
|
So this is a case where I start to doubt your judgement a bit because you say:
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.
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, |
That's not a micro-optimization. Your 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 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. |
packages/embla-carousel-angular/src/components/embla-carousel.directive.ts
Outdated
Show resolved
Hide resolved
packages/embla-carousel-angular/src/components/embla-carousel.directive.ts
Outdated
Show resolved
Hide resolved
packages/embla-carousel-angular/src/components/embla-carousel.directive.ts
Outdated
Show resolved
Hide resolved
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, |
Update
|
any chances to get inside contributors list?:)) |
I agree, but it's up to @davidjerleke . |
@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, |
Special thanks section is already very pleasant for me. Thanks! |
21d62d1
to
fde1f63
Compare
fb3c059
to
276ee85
Compare
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.
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 embla
packages 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
packages/embla-carousel-docs/src/content/pages/get-started/angular.mdx
Outdated
Show resolved
Hide resolved
packages/embla-carousel-angular/src/components/embla-carousel.directive.ts
Outdated
Show resolved
Hide resolved
262a043
to
d20c38d
Compare
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, 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
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 |
@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 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
What do you think? Best, |
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 : ) |
Hi @davidjerleke , Repo: https://github.com/donaldxdonald/embla-carousel-angular |
@donaldxdonald that was fast. Nice work, I think it looks great 🚀! You just got your first ⭐.
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, |
I think it's a great idea. Thanks @davidjerleke ! |
Thanks for all your efforts here @donaldxdonald, @zip-fa and @JeanMeche! |
Co-authored-by: David Jerleke <[email protected]> Co-authored-by: donaldxdonald <[email protected]>
d20c38d
to
cf6a6ff
Compare
@donaldxdonald I think this is cool: The first two users of I've added the link to the Angular wrapper now: Side noteIf you have a Reddit account you can share the news that |
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-angular
is packaged using the Angular CLI, which outputs the build to thedist
folder (or some other non-root path).All other libraries in the
embla-carousel
repository are packaged with rollup and exported directly to the root directory of the library, and then thenpm publish
command is executed in CI/CD, which is not directly usable inembla-carousel-angular
.So I've changed the release command in
cd.yml
to runyarn release
so 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.