Add SVG assets to bundle

fredericbonnet commented 5 years ago

This PR partly addresses #4 :

Flag SVGs are no longer pulled off GitHub but instead are distributed as assets along the main bundle
All flags are gathered in a flags module (under ./flags)
Each country is in its own flags submodule (e.g. flags/NL) to allow for eventual dynamic import/tree shaking
Flag module code is generated automatically from the country list

Flag assets are built using the Rollup plugin rollup-plugin-smart-asset in copy mode. This plugin also supports inline mode where data is wrapped as base64 into a JS module just in case it works better with tree shaking.

I've tested the code with angular-flagpack and the flag assets get correctly copied along the app bundles. No tree shaking for now but everything is in place to make it work.

Note: There are a handful of flags missing from the repo, I've commented them out in flags/index.js. Calling generateFlags.js again then re-building will fail with missing file errors.

zoeyfrisart commented 5 years ago

Hey, I wanted to first thank you so much for your contribution!

The team will look at it soon, and review the code to make sure its up to the standard and to make sure we can guarantee the quality and safety to the end users.

noudadrichem commented 5 years ago

@yannick1691 Do you want me to pick this up? Not sure if Donovan told you some stuff about what I said to him.

zoeyfrisart commented 5 years ago

@noudadrichem don’t think so, but feel free to pick this up! 👍

noudadrichem commented 5 years ago

So my general opinion on this solution is that it's pretty straight forward and works as expected. It also gave me new ideas on how to approach such problems so thank you for that! @fredericbonnet . What I'm thinking is that it's very redundant to have both SVG files generated again and JS files generated. In my opinion it's less redundant and cleaner to map the SVG code into the JS files itself. But i'm not sure how this would work in frameworks. I think we could publish this solution. But some todos and things to think about are definitely these points PLUS tree shaking. If I'm using 1 flag and build my test app it ships all files.

Do you have any experience with building tree shaking available packages? @fredericbonnet

Yet again thanks and I'll publish this as another step forward!

fredericbonnet commented 5 years ago

Hi! I've updated my PR with new code. Here are some explanations.

I've taken inspiration from Font Awesome regarding module structure and tree shaking, as they have exactly the same kind of problems to solve with their large SVG libraries. So now the core code and the SVG modules are separated in distinct modules, and you have to register the flags you want to use explicitly. This is needed if you want to reduce the final bundle size, because it is not possible to perform tree shaking on dynamic modules reliably. In your case, there is no way for the bundler to know which flags the package consumer will use because the country code is a simple string and not a module.

To illustrate, see the following page at Font Awesome's: https://fontawesome.com/how-to-use/with-the-api/other/tree-shaking

This means that your flagpack-core is now split into a main 'core' package and a 'flags' library package, the same way Font-Awesome uses @fortawesome/fontawesome-svg-core and @fortawesome/free-solid-svg-icons for example.

Which leads to the second part of my explanations.

I see you have several related npm packages;

flagpack-core: the core library
angular-flagpack: Angular bindings
react-flagpack: React bindings
vue-flagpack: Vue bindings

With the new 'flags' library this would give 5 packages.

The recommended practice with NPM in this case is to use scoped packages, see: https://docs.npmjs.com/misc/scope

This is not very complicated to implement in practice, it's just a naming convention. Simply rename your existing packages with the scoped syntax (editing the package.json is enough), republish them, and you're good. Let's say your scope is @flagpack, so:

flagpack-core becomes @flagpack/core
The new flags library will be @flagpack/flags
angular-flagpack becomes @flagpack/angular
react-flagpack becomes @flagpack/react
vue-flagpack becomes @flagpack/vue

For now both the core and flags modules are in the same repository and built in the same dist subdir, but they need to be in separate directories with their own package.json for scoped packaging to work. If you want to keep both packages in the same git repository (it's called a monorepo) then you can use Lerna for example: https://github.com/lerna/lerna

At present tree shaking works with the pushed code, but the syntax is a bit verbose. For example, if you want to include the whole flag library, just add this code to your app entry point (I've only tested it with Angular but it should work the same elsewhere):

import {registerFlagLibrary} from 'flagpack-core'
import * as flags from 'flagpack-core/dist/flags';
registerFlagLibrary(flags);

The final bundle size will be the same as with the previous code. Now if you only want to include specific flags, for example Germany and France:

import {registerFlagLibrary} from 'flagpack-core'
import {DE, FR} from 'flagpack-core/dist/flags'
registerFlagLibrary({DE, FR});

In theory this should only include the specified files though tree shaking, in practice I couldn't get it to work with my test app but I suppose it's an issue with WebPack (this may work with Rollup but I don't know this tool very well unfortunately). In that case, just use the alternate "deep import" syntax just like with Font-Awesome:

import {registerFlagLibrary} from 'flagpack-core'
import {DE} from 'flagpack-core/dist/flags/DE'
import {FR} from 'flagpack-core/dist/flags/FR'
registerFlagLibrary({DE, FR});

This syntax works for me, the bundle only includes the required flags.

You'll notice that I refer to the dist/flags subdir explicitly, that's because the flags library is still part of flagpack-core. Once scoped packaging is in place, the code will look like this:

// Core package
import {registerFlagLibrary} from '@flagpack/core'

// Whole flags package
import * as flags from '@flagpack/flags';
registerFlagLibrary(flags);

// Tree-shaking
import {DE, FR} from '@flagpack/flags'
registerFlagLibrary({DE, FR});

// Deep imports
import {DE} from '@flagpack/flags/DE'
import {FR} from '@flagpack/flags/FR'
registerFlagLibrary({DE, FR});

One last word about including SVGs as data URIs within JS. I suggest against this practice, because some frameworks like Angular require specific handling for security reason:

https://angular.io/guide/security#bypass-security-apis

Here we would need DomSanitizer.bypassSecurityTrustUrl() for SVGs as data URIs to work.

Besides, you get extra benefits with using separate SVGs: they compress better, can be loaded asynchronously by the browser in a separate thread, can be served off a CDN, etc.

Yummygum / flagpack-core

Add SVG assets to bundle #5