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

Discussion: API to register icons #3

Closed
devoto13 opened this issue Jan 14, 2018 · 25 comments · Fixed by #168
Closed

Discussion: API to register icons #3

devoto13 opened this issue Jan 14, 2018 · 25 comments · Fixed by #168
Milestone

Comments

@devoto13
Copy link
Collaborator

devoto13 commented Jan 14, 2018

I think we should discuss how API for the icon registration in the library should look like. If we should have it at all.

Option 1: reuse fontawesome.library

This means that users will have to do somewhere (where?) in their code:

import fontawesome from '@fortawesome/fontawesome'
import { faUser } from '@fortawesome/fontawesome-free-solid'

fontawesome.add(faUser);

Than they can just specify icon name in the template:

<fa-icon icon="fas-user"></fa-icon>
  • pro: No extra implementation in the angular-fontawesome library
  • con: Component using icons does not declare its dependencies clearly
  • con: Requires use of low level API
  • con: Not clear where to put this code

Option 2: provide helper method on the FontAwesomeModule

We'll provide a helper method, which will register icons passed in as parameters in the library. Here is a small example how it can look like:

import { faUser } from '@fortawesome/fontawesome-free-solid'

@NgModule({
  imports: [
    FontAwesomeModule.withIcons(faUser),
  ]
})
class AppModule {}

Than icon can be used in the templates by just providing its name like in option 1:

<fa-icon icon="fas-user"></fa-icon>
  • pro: Obvious place for registering icons
  • pro: Implementation is quite simple
  • con: Component using icons does not declare its dependencies clearly (better than option 1, but still)

Option 3: no registration at all

Require developer to import icon and pass it into template. In this case developer will have to import every icon they want to use in the component explicitly. Here is an example:

import { faUser } from '@fortawesome/fontawesome-free-solid'

@Component({
  selector: 'my-app',
  template: '<fa-icon [icon]="faUser"></fa-icon>',
})
export class AppComponent {
  faUser = faUser;
}
  • pro: Explicit dependency declaration for a component
  • pro Easy to find all icon usages or unused icons with TypeScript tooling (find usages, noUnusedVariables in TsConfig)
  • con: Requires extra field in component for every icon, quite verbose

Let me know what do you think?

@Shinigami92
Copy link

I as a developer would see 1 or 2 as the right way for DX

  1. has the advantage that the documentation remains very similar
  2. can be a bit messy for many icons

@mlwilkerson
Copy link
Member

@devoto13 thanks for the comments. One of our goals with the various official @fortawesome scoped JS framework components has been to maintain some consistency across them about how it feels to dev with them (while of course allowing for the distinctiveness of each of those frameworks). So just as a high level non-functional requirement for our design of the Angular component's API, bear that in mind.

I think what you've just laid out as the options above have some similarity (thought not identical) to the dev experience we've documented and more clearly over on the React component. See the discussion there about the Explicit, Library, and External approaches.

I offer that link in part to show that, over in the React and Vue worlds, the design decisions were made to enable at least those three different scenarios (and document them as such), since there may be reasonable use cases for each.

So my default approach for Angular would have been to bring across the same sort of approach: enable those which may have reasonable use cases, while letting devs know what trade-offs there may be for their choices of usage.

If you haven't already, could you absorb that React README (just the parts regarding those three usage scenarios) and let me know how or whether that influences your thinking about API recommendations for this Angular component?

@devoto13
Copy link
Collaborator Author

devoto13 commented Jan 14, 2018

I've read the React component README and I think that it is very similar to what we should do for Angular. Couple of notes:

Explicit

This is same as option 3 in the original post.

Library

This is option 1 or option 2 from the original post. Just want to note here that having global state (library) is not very common in Angular world, so that's why I offered option 2 at the first place. To host icons library inside NgModule and use this library when rendering icon instead of global one.

But option 2 can be just wrapper of the library.add method and add icons to the global library. The benefit is that the place where developers add icons to the library is clearly defined, so they don't need to guess where to put this code.

As for the concern @Shinigami92 raised about being messy with many icons. One of the solutions is to have a separate file to collect all icons and then add them. Something like this maybe:

// icons.ts
export { faUser } from '@fortawesome/fontawesome-free-solid'
export { faClone } from '@fortawesome/fontawesome-free-solid'
export { faCopy } from '@fortawesome/fontawesome-free-solid'
export * from '@fortawesome/fontawesome-free-brands'

// module.ts
import * as icons from './icons'

@NgModule({
  imports: [
    FontAwesomeModule.withIcons(icons),
  ]
})
class AppModule {}

External

Embedding Angular is not supported now, but some people do it and it will probably be supported at some point. IMO this is less important use case specifically for Angular.

@mlwilkerson
Copy link
Member

Now that we've got the first pre-release out there, I'm coming back around to thinking about this issue.

I do still want to have the Explicit / Option 3 approach available. So let's keep that.

Looking again at the idea of registering an icon in the Library, in my reading of the code, the Library isn't global, it's scope to a particular instance of the Library class internally. When fontawesome is loaded, an instance of Library is created and that's the one that would be in use when calling icon() or findIconDefinition(). So it isn't global, per se, though it is "relatively global", in the sense of being like a singleton instance that's used by other API calls.

Does that make any difference in your thinking, @devoto13, about wanting to avoid global state?

@devoto13
Copy link
Collaborator Author

devoto13 commented Jan 19, 2018

I do still want to have the Explicit / Option 3 approach available. So let's keep that.

It will probably come to just including FA bundle in a <script> tag. But we'll need to test that replacing icons from outside doesn't break Angular rendering. Think about <i class="fa fa-spinner fa-spin" *ngIf="isLoading"></i> for example. We should make sure that replacing icon won't break ngIf logic. (And it most likely will.)

RE library and global state. I will need to check the Library implementation in the base FA package to say if it makes sense to try to integrate it with Angular. I'll try to look into it tomorrow.


When integrating FA 5 into one of the projects I came up with pattern to make option 3 less verbose, but it has a disadvantage that IDE/linter are not able to detect unused icons anymore. Here is an example:

import { faUser, faSpinner, faChevronLeft } from '@fortawesome/fontawesome-free-solid'

@Component({
  selector: 'my-app',
  template: '<fa-icon [icon]="icons.faUser"></fa-icon>',
})
export class AppComponent {
  icons = { faUser, faSpinner, faChevronLeft };
}

It is more concise than original suggestion when using multiple icons in the component.

But I have to accept that it was pretty annoying to add all icons to the component first and only after that being able to use them in the template. Partially because auto-import didn't works as I described in #1 (comment).

I have a feeling that if we do packaging right it will be possible to do:

import * as icons from '@fortawesome/fontawesome-free-solid'

@Component({
  selector: 'my-app',
  template: '<fa-icon [icon]="icons.faUser"></fa-icon>',
})
export class AppComponent {
  icons = icons;
}

And still have tree-shaking remove all unused icons with Angular AoT compilation. But I'll need to test it. If it works it should be a great option, since developer won't have to import every single icon manually and they will still not bloat bundles with unused icons.

@FriOne
Copy link

FriOne commented Jan 20, 2018

Thinks we should have both options: #2 and #3. I guess we already have option #3, but #2 is very familiar to angular users.
How about to setup aliases for icons? For example I have:

export { faUser } from '@fortawesome/fontawesome-free-solid'

@NgModule({
  imports: [
    FontAwesomeModule.withIcons({
     'user': faUser,
   }),
  ]
})
class AppModule {}
<fa-icon icon="user"></fa-icon> 

And then at some point I want to change it to user-circle, so I I just change:

export { faUserCircle } from '@fortawesome/fontawesome-free-solid'

@NgModule({
  imports: [
    FontAwesomeModule.withIcons({
     'user': faUserCircle,
   }),
  ]
})
class AppModule {}

And it will be changed everywhere.

@karptonite
Copy link

karptonite commented Feb 12, 2018

We are discussing moving our Angular 5 app to Font Awesome from a combination of Glyphicons and Foundation icons, so this is very interesting to us!

For a team like ours, where we have a designer working somewhat separately from the programmers, Option 1 and Option 2 are appealing. The designers can just work in the templates, and doesn't have to poke around in the component.ts files, as they would have to with Option 3. Option 2 is definitely the most consistent with the way other Angular libraries seem to work.

It might be worth thinking about how that will work with Lazy-loaded modules. Will there be issues if multiple lazy loaded modules try to add the same icon? Should all icons be added in root, or can we use lazy loading in each feature module? If there are scoping issues with the library, what determines which version of the library a template has access to? One way to make the answers to some of these questions obvious is to use for a conventional name for the import function, FontAwesomeModule.forFeature() and/or forRoot() instead of withIcons().

Regarding option 1: @devoto13 wrote

This means that users will have to do somewhere (where?) in their code

I have the same question: where? We are thinking of starting in with the prerelease version of this using Option 1, but I'm really not sure where something like this belongs in an Angular application.

@FriOne
Copy link

FriOne commented Feb 13, 2018

@karptonite I guess the icons will be in main module only in case of lazy loading. Because modules separated by webpack.

@devoto13
Copy link
Collaborator Author

For a team like ours, where we have a designer working somewhat separately from the programmers, Option 1 and Option 2 are appealing. The designers can just work in the templates, and doesn't have to poke around in the component.ts files.

@karptonite Note that having a library with all icons included will retain all of this icons in production bundle, which can be a significant amount of bytes. The only option, which currently provides a reliable tree shaking is option 3.

@karptonite
Copy link

@devoto13 I'm not considering a library with all icons included. However, importing all of the icons that we will be using on the site in one place seems like a reasonable tradeoff, and imports only the icons used (although it currently requires deep imports). I don't see how that is incompatible with option 2 if you only import the icons used there as well.

@devoto13
Copy link
Collaborator Author

devoto13 commented Feb 21, 2018

@karptonite Yup, if you import only icons you use, it is fine. But it also means that designer will have to import icons and add to the library, when they want to use a previously unused one. It is very specific to your workflow if this is acceptable trade-off. I just wanted to point out that including all icons in the library is a bad idea 😄

@karptonite
Copy link

@devoto13 Yes, that is true. Importing in one place for each icon is an acceptable workflow; importing in each component separately and assigning to a variable in the component is not.

@FriOne
Copy link

FriOne commented Feb 22, 2018

@karptonite you can create a pipe, in that case you will have only one entry point.

@karptonite
Copy link

@FriOne that is intriguing. Are you proposing a | icon pipe that takes a string or array representation of an icon and returns the icon itself? You could do all the imports in the pipe. That could work, but I don't see how it gains you much over registering the icons directly with option 1 or option 2.

@robmadole
Copy link
Member

Wanted to pop in here and mention that the "encapsulation" of Option 3 (and 2 as well) will provide better long-term scaling as projects and teams grow. One of the issues that has always bothered me about the fontawesome.library is that it creates a dependency between the component and the thing that sets up the library. This can create an opportunity for sneaky things to break (what if someone accidentally deletes some icons in a particularly tricky git merge?)

So while the library provides convenience we need to realize that the convenience will come at a price. I personally have worked on enough projects that the temporary pain of being explicit always out-weighs the chance of things getting messy and error-prone in the future.

@mlwilkerson
Copy link
Member

Now with 0.1.0-8 pre-released, which supports all of the Font Awesome API features, I'm circling back around here to this issue to re-assess where we may want to go from here.

I think I'm hearing a consensus that @devoto13's Option 2 above is the most Angular-friendly approach, retains good encapsulation and is more scalable as teams grow. Would look like this (updated icon pack name in light of the FA 5.1 re-packaging scheme):

import { faUser } from '@fortawesome/free-solid-svg-icons'

@NgModule({
  imports: [
    FontAwesomeModule.withIcons(faUser),
  ]
})
class AppModule {}

@devoto13 mentioned a concern about tree-shaking: that only Option 3 (explicit) would support tree-shaking. But I have a feeling that this Option 2 would also support tree-shaking, and I'd be interested in seeing someone work up a PR to demonstrate it. If tree-shaking does work with Option 2, then it sounds like we'd have a winner with Option 2.

It seems like Options 1 and 3 remain available to developers, because they're simply uses of JavaScript and/or Font Awesome's library to approach things in a different way. But Option 2 could be the recommended and documented best-practice approach.

Also, as I was reviewing this thread, I noticed @devoto13's quasi-Option 4 which was:

I have a feeling that if we do packaging right it will be possible to do:

import * as icons from '@fortawesome/fontawesome-free-solid'

@Component({
  selector: 'my-app',
  template: '<fa-icon [icon]="icons.faUser"></fa-icon>',
})
export class AppComponent {
  icons = icons;
}

I tried a couple of variations on that theme using 0.1.0-8 and neither of them shook the tree.

  1. import { fas } from... and then <fa-icon [icon]="fas.faUser">
  2. import * as fas from... and then <fa-icon [icon]="fas.faUser">
    They both result in all of the fas icons included in the production bundle.

Another [comment I noticed above], from @FriOne, was the idea of fixing a string in the template to represent an icon, like:

<fa-icon icon="user"></fa-icon> 

But then (using the Option 2 pattern) being able to change from something like this:

    FontAwesomeModule.withIcons({
     'user': faUser,
   }),

to something like this:

    FontAwesomeModule.withIcons({
     'user': faUserCircle,
   }),

...effectively keeping the same icon name in the template, but changing the backing icon object.

I have a concern about that: it implicitly creates a different icon naming scheme than what you'd find in the icon catalog on fontawesome.com. In our catalog, the icon name "user" always refers to the faUser icon. Whereas faUserCircle has its own iconName: "user-circle".

Those icon names have Font Awesome API significance, which would be obscured if a template ended up looking like it was using the icon name "user" but rendering the faUserCircle icon instead of the faUser icon. I anticipate the support requests coming in: why is faUserCircle showing up, when I told it to use icon="user"?

So I feel reluctant about that proposal.

Shall we run an experiment with an Option 2 implementation and have a look at its tree-shaking properties?

@karptonite
Copy link

How would Option 2 work when importing icons with the same name from different sets? Say you want to use the same icon from the solid and the light sets in different places?

@mlwilkerson
Copy link
Member

@karptonite yes, good point.

We currently support this syntax for specifying the icon prefix (aka "set", "style"):

<fa-icon [icon]="['far','user']"></fa-icon>

Would that suffice?

Another option would be to add a prefix attribute to the component, which is what we did over on our Ember component (and I have a vague recollection that we've discussed that in this repo as well).

So if you need to disambiguate or specify a non-default prefix, you might do it like this:

<fa-icon prefix="far" icon="user"></fa-icon>

@robmadole
Copy link
Member

<fa-icon prefix="far" icon="user"></fa-icon>

has some complications when we start talking masks:

<fa-icon prefix="far" icon="user" maskPrefix="..." maskIcon="..."></fa-icon>

🤢

And there also other complications. I hashed over this in the Vue project here

(After using vue-fontawesome myself for building fontawesome.com the array syntax of [icon]="['far', 'user']" was the most usable in my opinion even if it was ugly and seemed gross at first exposure to it)

@mlwilkerson
Copy link
Member

Yeah, good point, @robmadole. I personally am satisfied with that rational for just sticking with the array syntax: that you've run this through its paces in a real world project using the breadth of features (including mask).

@karptonite
Copy link

karptonite commented Mar 28, 2018

@mlwilkerson that array form is fine for in the templates. I'm wondering how it would work in the imports.

import { faUser } from '@fortawesome/free-solid-svg-icons'
import { ????User } from '@fortawesome/free-light-svg-icons'


@NgModule({
  imports: [
    FontAwesomeModule.withIcons(faUser, ???),
  ]
})
class AppModule {}

would we have to use "as" syntax for all of our non-solid imports?

@mlwilkerson
Copy link
Member

mlwilkerson commented Mar 28, 2018

OK yes, I wondered if that might be a concern. And yes, I think that's normal that you'd have to use the as syntax so your JS object names don't collide. But that might be all you'd need to do, because those icon objects know what their own prefixes are.

So if you did this:

import { faUser as fasUser } from '@fortawesome/free-solid-svg-icons'
import { faUser as farUser } from '@fortawesome/free-regular-svg-icons'

@NgModule({
  imports: [
    FontAwesomeModule.withIcons(fasUser, farUser),
  ]
})
class AppModule {}

It would work, and you'd then just use the aforementioned array syntax in the templates to specify which you intend at each point in the template.

@karptonite
Copy link

Ok, that is similar to what we do now. It is a shame that different namespaces weren't used for the different sets, though. It makes it less convenient than it would otherwise have been.

@robmadole
Copy link
Member

robmadole commented Mar 28, 2018

@mlwilkerson do you think Angular could successfully tree shake this:

import { fas } from '@fortawesome/free-solid-svg-icons'
import { far } from '@fortawesome/free-regular-svg-icons'

@NgModule({
  imports: [
    FontAwesomeModule.withIcons(fas.faUser, far.faUser),
  ]
})

@mlwilkerson
Copy link
Member

I doubt it, because it wouldn't tree shake when I tried:

import { fas } from... in the module and then <fa-icon [icon]="fas.faUser"> in the template, which seems equivalent from a tree-shaking perspective.

@devoto13 devoto13 added this to the 0.5.0 milestone Aug 9, 2019
devoto13 added a commit to devoto13/angular-fontawesome that referenced this issue Aug 11, 2019
Usage:

```
export class AppModule {
  constructor(library: FaIconLibrary) {
    library.addIcons(faCoffee);
  }
}
```

Using DI instead of the syntax discussed in the issue
(`FontAwesomeModule.withIcons(faUser)`) as this is more flexible and
future proof given that future version of Angular renderer (Ivy) is
going make usage of modules optional.

---

Deprecated using library from `@fortawesome/fontawesome-svg-core` in
favor of the new class. Previous library was problematic in several
ways:

- global variable, which was shared by all code on the page
- more complicated for consumers as they need to know about existence of
`fontawesome-svg-core` and that it is used by `angular-fontawesome`
- library from `fontawesome-svg-core` implementation was pretty complex

This deprecation is the first step on the way to make
`fontawesome-svg-core` an implementation detail of
`angular-fontawesome`, which consumers don't need to be aware about.

Fixes FortAwesome#3
devoto13 added a commit that referenced this issue Aug 11, 2019
Usage:

```
export class AppModule {
  constructor(library: FaIconLibrary) {
    library.addIcons(faCoffee);
  }
}
```

Using DI instead of the syntax discussed in the issue
(`FontAwesomeModule.withIcons(faUser)`) as this is more flexible and
future proof given that future version of Angular renderer (Ivy) is
going make usage of modules optional.

---

Deprecated using library from `@fortawesome/fontawesome-svg-core` in
favor of the new class. Previous library was problematic in several
ways:

- global variable, which was shared by all code on the page
- more complicated for consumers as they need to know about existence of
`fontawesome-svg-core` and that it is used by `angular-fontawesome`
- library from `fontawesome-svg-core` implementation was pretty complex

This deprecation is the first step on the way to make
`fontawesome-svg-core` an implementation detail of
`angular-fontawesome`, which consumers don't need to be aware about.

Fixes #3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants