bencodezen / vue-enterprise-boilerplate

An ever-evolving, very opinionated architecture and dev environment for new Vue SPA projects using Vue CLI.
7.78k stars 1.32k forks source link

Update vue-fontawesome to 0.1.0 #63

Closed radum closed 5 years ago

radum commented 6 years ago

I've updated vue-fontawesome to 0.1.0 following their migration docs

What I'm not sure about is this part:

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

Before this change require was used to separate them down bellow the code. IMHO mixing require and import might not be such a good idea. I get the point why require had to be used as imports need to be at the top.

But I think might be better if we import the icons on top and use them in our array later.

frandiox commented 6 years ago

@radum Thanks for the update. I believe these changes rely on tree shaking to drop other icons since all of them are initially imported when doing import { faSync, faUser } from '@fortawesome/free-solid-svg-icons'. Tree shaking probably works well already in Webpack but it may be slow.

Using deep imports like this should solve that issue (and is more similar to the original code): https://github.com/frandiox/vue-graphql-enterprise-boilerplate/commit/868b3c67e66805159fbd5d2d7b06de7ace23bb3c

Let's see what @chrisvfritz says about it 👍

radum commented 6 years ago

Hmm, you might be right @frandiox I will double check to see how they work and if the tree shake works properly.

chrisvfritz commented 5 years ago

I found another way to implement this which keeps things fast and lean. Thanks for the help! 🙂

rickyruiz commented 5 years ago

@chrisvfritz How about doing something like this:

import { library, IconPack } from '@fortawesome/fontawesome-svg-core'
import { FontAwesomeIcon } from '@fortawesome/vue-fontawesome'
import Vue from 'vue'

const requireIcon = require.context(
  '@fortawesome/free-solid-svg-icons',
  false,
  // Add new icons to this list as you need them (https://fontawesome.com/icons)
  /fa(sync|user|caretDown|tachometerAlt|infoCircle|clock|fire).js/i
)

requireIcon.keys().forEach((fileName: string) => {
  // Get icon definition
  const { definition }: IconPack = requireIcon(fileName)

  // Add icon definition to FontAwesome library
  library.add(definition)
})

Was going to create an icon array to make it more readable but apparently require.context regex needs to be statically analyzable: https://github.com/webpack/webpack/issues/4772

chrisvfritz commented 5 years ago

@rickyruiz That's another way to do it! 🙂 I think I'll stick with what I have for now though. Otherwise, anyone who wants to add an icon will be required to have some level of comfort with both require.context and regex. I personally try to only use require.context where it's unlikely to ever need to be modified, to keep things easier for teams of varying skill levels.

rickyruiz commented 5 years ago

@chrisvfritz Aside from what you pointed out, jest complains about require.context , they actually discourage using it in components. I'll stick with what you have as well 👍.