Why not an npm module? - Githubissues

FriOne commented 6 years ago

I see repos for react and vue, but not for angular. Do you have problems with create one? You could do it with this https://github.com/gonzofish/angular-librarian https://github.com/FriOne/ng-fontawesome

mlwilkerson commented 6 years ago

@FriOne I'm actively working on completing the Angular component to make available via npm soon.

devoto13 commented 6 years ago

@mlwilkerson Is there anything I can do to help move this forward?

mlwilkerson commented 6 years ago

@devoto13, thanks for offering. I didn't expect it to be 13 days later from my last comment and still not have this component shipped. What's happened in the interim is that I realized that we needed more attention and testing on the TypeScript definitions, which are a foundational piece of enabling the Angular development experience.

So TypeScript has been my focus recently, even re-working our library's exports in order to better accommodate TypeScript development. What I think will be our final PR for that work is under review now.

So, let me think, if you were to help it move forward, it might be to submit a PR that is either:

(A) A fix of the Angular QuickStart Lib implementation I have tried to get working on the library-reorg branch (I just pushed my latest commits there). I think the last problem had to with getting yarn integration (or one of those) to work due to something about Karma's loading of modules via SystemJS and the configuration thereof in karma-test-shim.js. But my memory is fuzzy—or I might have blocked it out.

or

(B) A a proof of concept that instead uses Angular Librarian (as mentioned by @FriOne above)

and

in either case, also makes use of the latest pre-releases I've put out:

@fortawesome/fontawesome@1.1.3-2
@fortawesome/fontawesome-free-solid@5.0.5-1

They now have comprehensive typings (though there may be one more breaking change to come).

As a side quest, you could scrutinize the type definitions in those packages and the dependency they'll pull in, @fortawesome/fontawesome-common-types, and respond with any suggestions for improvement, especially as anything that turns up in the process of trying to use them in an Angular environment. (I do already have passing integration tests for them with an Angular 5 angular-cli app.)

Some background:

After making a proof of concept angular-cli app, I had initially attempted to library-ize this component using Angular QuickStart Lib. It was my initially preferred approach after reading about it, Angular Librarian, and ng-packagr.

But there were those knots with the QuickStart that I had not managed to untangle before I realized that I needed to lock down the TypeScript definitions anyway.

So one of my next steps after nailing down the TypeScript was going to be to explore the trade offs of using Angular Librarian. I'm not 100% convinced that it's the way for us to go, but that was to be my next step. But if you know how to fix what's broken with the QuickStart-based approach in the library-reorg branch now, perhaps that would be quicker. My ultimate preference would be to use whatever library support angular-cli ships as built-in in the future, but that's the future.

mlwilkerson commented 6 years ago

@devoto13 Wait, I have another idea: we could bypass (temporarily) some of the library packaging refinements and I could give you a pre-release npm of the repo in its current state. You could start using in your Angular development, and giving feedback to help me iterate and finish it up for a full release.

I went back to try and refresh my memory about the state of things on that branch and to go ahead and update its dependencies to my latest pre-releases, and was reminded that the core functionality of the component is already working. It's the test running frameworks (unit and integration) that aren't quite building a loading properly. But the demo app (i.e. yarn start) demonstrates that the basic component functionality is working (with just a couple of tweaks to update it to be compatible with the pre-releases).

How does that sound?

devoto13 commented 6 years ago

Yes, let’s do it. I planned to spend some time on Sunday to investigate unit testing problem you reported. I can try to integrate NPM pre-release first and than look into unit tests if I have some time left.

devoto13 commented 6 years ago

@mlwilkerson I've checked those 3 tools for building Angular libraries and I would say that https://github.com/filipesilva/angular-quickstart-lib is problematic in 3 areas:

It's quite abandoned - no activity for almost half a year.
It does not support scoped packages according to their README (i.e. not possible to publish package like @fortawesome/angular-fontawesome, only angular-fontawesome).
It uses System.js. In practice working with Webpack is much simpler (unless you have to configure everything yourself :D).

The other two seems to be more or less on the same level regarding developer experience and supported features. So I have prepared proof of concept using Angular Librarian (since you mentioned this before). I've tested it against my project and everything seems to work well. It is almost same as what @FriOne linked in the initial comment, but with slightly different naming and in a form of PR, which can be merged "as is".

There is however much bigger problem we should solve before we can ship this library. And it's related to how @fortawesome/fontawesome and icon packs are packaged on NPM. I have touched on that in https://github.com/FortAwesome/Font-Awesome-Pro/issues/978, but haven't heard back for couple of days. I really think that we should come up with feasible solution there as soon as possible, before FA5 is widely adopted. Proper solution most likely will require introduction of breaking changes.

In regards to Angular having side effects and global state creates two problems:

When users include FontAwesomeModule into their application it will break all usages of FA4 icons without any apparent reason for that to happen. It is a side effect of icon.component.ts importing @fortawesome/fontawesome, which attempst to replace <i class="fa fa-user"></i> with SVG icons. Such side effects are not expected by Angular users and should be avoided.
Currently NPM packages for icon packs come with side effect of registering ALL icons in the fontawesome.library. This is horrible for the tree shaking, which is key feature of Angular. The innocent import of import { faUser } from '@fortawesome/fontawesome-free-solid' will lead to hundreds of KBs of unused icons to be included in the bundle because of this side effects. I believe I came up with feasible solution at the end of https://github.com/FortAwesome/Font-Awesome-Pro/issues/978#issuecomment-356819758. Can you take a look and share any feedback?

Assuming that we resolved previous 2 issues we should also think about how we should implement "icon registration" process (if any). I've opened a separate issue to discuss it (https://github.com/FortAwesome/angular-fontawesome/issues/3).

mlwilkerson commented 6 years ago

@devoto13 Thanks!

RE: QuickStart, I don't feel devoted to it. But part of what inclined me that way is that it was an Angular-CLI team member who built it, apparently to demonstrate what might be the way forward in how angular-cli might support libraries in the future. So I didn't expect the QuickStart thing to be maintained itself but rather inferred that it might be a pattern that would have future support in angular-cli. But I acknowledge those weaknesses, and at the end of the day, it's not working, so let's do what works. (It's comment about lack of support for scoped packages is strange, though, since the @angular packages it uses are all scoped, and our @fortawesome packages work, except for the test framework. But, whatever.)

RE: Angular component's impact to FA4 icons and such, I think we've already fixed that recently in our private repo, but it hasn't yet been released to npm. I noticed the same sort of interaction with React components. So the idea is that one of these component's use of the framework should not disable the autoReplace functionality that translates <i> tagged icons into <svg>. Is that the same issue you're highlighting here? If not, maybe we need to get another issue logged.

I just now scanned the discussion you linked to at #978. I think we're concerned about the same sorts of things (though you've thought more deeply about it than me—I'm still catching up), since we've been discussing it internally. The tree-shaking concerns you raise, for example. So it's not all resolved yet, but I can assure you that we're paying attention to these matters. Your analysis and feedback is really helpful.

I'll go and comment on #3 as well.

devoto13 commented 6 years ago

@mlwilkerson I totally understand your motivation when choosing QuickStart.

(It's comment about lack of support for scoped packages is strange, though, since the @angular packages it uses are all scoped, and our @fortawesome packages work, except for the test framework. But, whatever.)

Angular itself uses quite custom and ad-hoc build process, so probably not a good example :) In any case what is valuable is what lies in src and example app from examples folder. Everything else is pile of chore configurations to get tools working and it's is not much hassle to replace them with another configurations. Took me couple of hours to replace QuickStart Lib with Librarian. I don't think it will become a problem to switch to Angular CLI when it ships library support (and I don't expect this to happen soon).

RE: Angular component's impact to FA4 icons and such, I think we've already fixed that recently in our private repo, but it hasn't yet been released to npm. I noticed the same sort of interaction with React components. So the idea is that one of these component's use of the framework should not disable the autoReplace functionality that translates <i> tagged icons into <svg>. Is that the same issue you're highlighting here? If not, maybe we need to get another issue logged.

Yes, that's the exact issue I'm experiencing. But I think that the bundle should not have this side effect at the first place and than the solution of disabling it when using component won't be needed at all. This is what I'm after in https://github.com/FortAwesome/Font-Awesome-Pro/issues/978 - to make importing/loading of FA module or icon pack to not have any side effects. This will ultimately solve tree-shaking issue, auto replace icons issue and will allows to get all of these when importing from default entry point without any special build tools configuration. I'll try to get a more structured write-up of what I'm after tomorrow.

And I'm happy that you guys listen and act on the users feedback. I think it is very important for the project to succeed.

devoto13 commented 6 years ago

@mlwilkerson what would be the next step here? I think I will have couple of hours in the coming days, so can make a prototype of the icons library implementation described here and/or add some tests to the repo.

mlwilkerson commented 6 years ago

UPDATE: @fortawesome/angular-fontawesome@0.1.0-1 is now available as an npm pre-release.

devoto13 commented 6 years ago

I have migrated project to the pre-release version and it works very well. Great job! I think it will be very smooth experience after packaging is improved.

So if I import 2 icons to use in my application from the main, here is how my bundles looks like:

import { faTimes } from '@fortawesome/fontawesome-free-solid';
import { faClone } from '@fortawesome/fontawesome-pro-light';

Note 770KB of the FA code added to the bundle. It is an effect of not having tree shaking out of the box and me not reading documentation :)

And here is how it looks if I use deep imports:

import { definition as faTimes } from '@fortawesome/fontawesome-free-solid/faTimes';
import * as faClone from '@fortawesome/fontawesome-pro-light/faClone'; // pro packages are not updated with type defs yet

Note that now FA takes only 30KB. I think it is a great improvement.

PS I don't quite like definition as faTimes part. What do you think about renaming definition/adding alias same as the icon name to icon files? So users will be able to do:

import { faTimes } from '@fortawesome/fontawesome-free-solid/faTimes';
import { faClone } from '@fortawesome/fontawesome-pro-light/faClone';

This should also work better with TypeScript auto import from completion popup and quick fix for adding missing import. I.e. when I write faChevronRight in the code TypeScript will autocomplete it and suggest to auto-import only from main now. But with named icon export it will include the deep import option as well.

Currently and after adding named export.

FriOne commented 6 years ago

@devoto13 is it possible to make tree shaking work with:

import { faTimes } from '@fortawesome/fontawesome-free-solid';
import { faClone } from '@fortawesome/fontawesome-pro-light';

?

Can't find any good article about it.

devoto13 commented 6 years ago

@FriOne Currently, you have to add special configuration for the bundler to enable tree-shaking when importing from main. See documentation.

Guys are working on changing packaging format, which should hopefully eliminate this step in the next release.

FriOne commented 6 years ago

Can it be written inside tsconfig.json paths?

{
  "compilerOptions": {
    "paths": {
      "@fortawesome/fontawesome-free-solid":' "../node_modules/@fortawesome/fontawesome-free-solid/shakable.es.js"
    },
}

I think config ejection is not so popular when you work with the CLI.

robmadole commented 6 years ago

@devoto13 this is pretty comical. Your examples:

import { faTimes } from '@fortawesome/fontawesome-free-solid/faTimes';
import { faClone } from '@fortawesome/fontawesome-pro-light/faClone';

...is something that @mlwilkerson also thought would be the most intuitive. But I tried very hard to talk him out of it.

My basic argument is that @fortawesome/fontawesome-free-solid/faTimes becomes a sub module and has more utility than just "give me the icon". For example if you just want the SVG path data you can do this:

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

It now has an API with named exports that all have meaning (width, height, svgPathData, unicode, ligatures, iconName) all available from it.

And while import { faTimes } from '@fortawesome/fontawesome-free-solid/faTimes' looks alright in ES6 it starts to get nasty in commonJS.

const faTimes = require('@fortawesome/fontawesome-free-solid/faTimes').faTimes

That duplication of faTimes').faTimes is going to be confusing and I can almost predict the titles of the many GitHub issues we will receive ("Remove faTimes faTimes duplication not needed")

That being said I'm happy to be outvoted on this one and let the majority rule. It just felt a little dirty to me as soon as I saw it.

devoto13 commented 6 years ago

@FriOne I think it will work. But can you try and post back the results?

devoto13 commented 6 years ago

@robmadole Yeah, does not look that nice in CommonJS... What do you think about keeping current exports structure, but adding one more export named same as icon? This should keep both ES2015 and CommonJS users happy. And I don't think there will be too much overhead from adding it.

E.g. add exports.faUser = exports.definition for every icon?

robmadole commented 6 years ago

@devoto13 it's low risk and I'm :+1: to add it. But I will never use it! 😆

@mlwilkerson you make the final decision on this please. Add that additional export.faUser ... if you support it.

mlwilkerson commented 6 years ago

Oh, I support it 😉 Coming right up!

mlwilkerson commented 6 years ago

Well, maybe not right up. I'm on bad wifi again at the moment...but soon.

mlwilkerson commented 6 years ago

Circumventing wifi...done. @devoto13 Now try icon pack pre-release 5.1.0-2. Should work for any of the -free- or -pro- svg icon packs, like

@fortawesome/fontawesome-free-solid@5.1.0-2

FriOne commented 6 years ago

@devoto13 I would like to but don't know how to install this module since it is not available on npm.

devoto13 commented 6 years ago

@FriOne oh, I just noticed that it was published as a private package (so only FA Pro subscribers can access it).

@mlwilkerson is it intended? Would be nice to have more people to try it out and give their feedback.

I tried to use paths. I've added below to the src/tsconfig.app.js in the fresh Angular CLI app:

    "paths": {
      "@fortawesome/fontawesome-pro-light": ["../node_modules/@fortawesome/fontawesome-pro-light/shakable.es.js"]
    },

And I put a shakable.es.d.ts next to the JS file, but I still get an error:

src/app/app.component.ts(2,48): error TS7016: Could not find a declaration file for module '@fortawesome/fontawesome-pro-light'. '/Users/devoto13/Projects/fa-test/node_modules/@fortawesome/fontawesome-pro-light/shakable.es.js' implicitly has an 'any' type.
  Try `npm install @types/@fortawesome/fontawesome-pro-light` if it exists or add a new declaration (.d.ts) file containing `declare module '@fortawesome/fontawesome-pro-light';`

I haven't used paths before, so not sure what is the problem here.

In any case this should be solved with new FA packaging.

devoto13 commented 6 years ago

@mlwilkerson Works like a charm with 5.1.0-2!

mlwilkerson commented 6 years ago

Was not intended to remain private. The default is private upon initial publication and I forgot to go back and change it to public. I’ll fix that when I can get back to my laptop.

Sent from my iPhone

mlwilkerson commented 6 years ago

UPDATE: it's now public

FriOne commented 6 years ago

@mlwilkerson It works, thanks. Will you update the documentation in part where you need to change config to make the module tree-shakable? I mean tsconfig.json, like this:

{
  "compilerOptions": {
    "paths": {
      "@fortawesome/fontawesome-free-solid": ["node_modules/@fortawesome/fontawesome-free-solid/shakable.es.js"],
      "@fortawesome/fontawesome-free-brands": ["node_modules/@fortawesome/fontawesome-free-brands/shakable.es.js"]
    }
  }
}

mlwilkerson commented 6 years ago

@FriOne I'd prefer to hold off on adding that documentation. We're in the process of removing the need for that shakable configuration—I expect it to be gone very soon. Also, we're still in a pre-release stage with this component—we expect more significant changes like this before it stabilizes and is ready to release.

FriOne commented 6 years ago

Great! The module is very helpful, thanks :+1:

mlwilkerson commented 6 years ago

@FriOne I changed my mind :wink: . I was updating the README to provide more context for project status and such for newcomers and went ahead to add your example tsconfig.json in the section on tree shaking. Thank you!

pantonis commented 6 years ago

I dont want to use a pre-release version. So I am using package.json to download the packages

      "@fortawesome/fontawesome": "1.1.4",
        "@fortawesome/fontawesome-pro-solid": "5.0.8",
        "@fortawesome/fontawesome-pro-regular": "5.0.8",
        "@fortawesome/fontawesome-pro-light": "5.0.8",
        "@fortawesome/fontawesome-free-brands": "5.0.8",

and then I am stuck to the angular-cli part. Do I need to reference something here? How do I use the icons?

devoto13 commented 6 years ago

@pantonis I would strongly recommend to either use pre-release version or wait until there is a first stable release. There are some instructions how to setup it with base library here, but they are not very clear. You'll also get much worse experience with this setup (bad docs, because it is not intended use), no tree-shaking (adds about 300kb to your application for every icon pack used) and performance problems (because base library is not optimized to be used with SPA's).

pantonis commented 6 years ago

Can I use the pro icons with webfonts with css?

robmadole commented 6 years ago

@pantonis yep, you sure can. Let me know if you need help with that.

mlwilkerson commented 6 years ago

@pantonis +1 to @devoto13's recommendation to use pre-release versions or wait. The development of this Angular component has actually been a key driver of the changes in our soon-forthcoming 5.1 release. So a lot about this Angular component working as designed, from the outset, depends upon what its in the pre-release. I think of the whole Angular component itself has really a pre-release, depending on pre-released bits in the core Font Awesome library.

My recommendation would be to use the pre-release versions for now, knowing that the corresponding official release is soon to follow. Completing the 5.1 release is a high priority for us right now.

pantonis commented 6 years ago

I think I managed to get it Fontawesome Pro webfonts with css working. For anyone else having the same problem (lack of documentation) this is what I did to use it with Angular 5 and angular-cli:

Use the instructions here to set fontawesome pro npm registry.
Run npm view @fortawesome/fontawesome-pro-webfonts to view the latest version
Open package.json and add "@fortawesome/fontawesome-pro-webfonts": version found on step 2

Open angular-cli.json and add the following lines under styles section

"styles": [
"../node_modules/@fortawesome/fontawesome-pro-webfonts/css/fontawesome.css",
"../node_modules/@fortawesome/fontawesome-pro-webfonts/css/fa-solid.css",
"../node_modules/@fortawesome/fontawesome-pro-webfonts/css/fa-regular.css",
"../node_modules/@fortawesome/fontawesome-pro-webfonts/css/fa-light.css",
"../node_modules/@fortawesome/fontawesome-pro-webfonts/css/fa-brands.css"
],

Compile and you have font awesome 5 available!!

mlwilkerson commented 6 years ago

Glad you got it working, @pantonis.

Some caveats:

(1) using this approach will end up undermining several of the key goals of this Angular component (such as providing an optimized and Angular-native way of working with icons), so I consider it a pretty substantial workaround, to the point that it's really an alternative to using this Angular component altogether.

(@pantonis Is that how you set up your build, I'm curious? Just cutting out the Angular component? And then use <i> tags with icon class names to reference the icons instead of the Angular component <fa-icon>?)

(2) For each of those .css assets the browser loads, it will also load the corresponding webfont asset with all of the icons for that style. So there's nothing like tree-shaking optimization available in that usage scenario.

(3) The @fortawesome/fontawesome-pro-webfonts package will be deprecated in the forthcoming 5.1 release. So while there will continue to be a way to accomplish the same objective (and we're working on all the documentation for that too!), the packaging will be different.

pantonis commented 6 years ago

@mlwilkerson right now I am in the middle of a release and as I said I cannot risk using a beta library. I have also several thousands of lines of code and I don't have the time to update them using the angular directive of the angular lib. It will cost me money.

By saying @fortawesome/fontawesome-pro-webfonts will be deprecated in the 5.1 you mean <i> tags will not be working anymore?

Thanks

mlwilkerson commented 6 years ago

@pantonis I understand. Sometimes we gotta make those calls. I also want others who find this to know that the path you've chosen for incorporating Font Awesome 5 in an Angular app is really a different implementation method than what we're trying to enable here with this component. I trust that you have good business reasons for doing so.

The method you've chosen is a perfectly legitimate use of Font Awesome using CSS with Webfonts. And we expect that there are plenty of cases where getting into Font Awesome 5, starting with CSS with Webfonts (instead of SVG with JS) is going to be the faster and smoother way to get going. And if you're using that method in Angular app, I would describe this approach as:

"Using CSS with Webfonts in my Angular app instead of using the @fortawesome/angular-fontawesome component in my Angular app."

The purpose of this @fortawesome/angular-fontawesome component is to provide an Angular-native and optimized means of implementing our SVG with JS usage method.

As for <i> tags, those are here to stay. Even when we deprecate the @fortawesome/fontawesome-pro-webfonts package, that would only mean that if you choose to keep using this method and want to get up to date with your packages, you'd need to follow the upgrade path we recommend with the release to use the new packages. But you wouldn't need to change anything in your templates with your <i> tags.

pantonis commented 6 years ago

Business reasons are the only reason I intend to use this method. I will add angular-lib to our backlog with high priority with a note to switch when angular lib is production ready.

FortAwesome / angular-fontawesome

Why not an npm module? #1