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

Convert addon to v2, add typescript & glint #239

Open
wants to merge 16 commits into
base: 2.x
Choose a base branch
from

Conversation

mkszepp
Copy link
Contributor

@mkszepp mkszepp commented Oct 26, 2024

This PR follows the new ember addon format RFC.
After this PR, the addon is a ember addon v2.
Most of the ember addons were already moved to v2, otherwise they will not anymore work in next gen of ember.

The move to the v2 addon is braking, because the addon is now a real npm package and magically parts of ember were removed. Removing magically parts of ember addons brings the effect that users must configure more things inside there app (like it is with all non ember-addons).
Converting to an v2 addon makes the addon itself easier, costs less maintaince, because the dependencies were reduced to a minium see
In additional we are moving away from broccoli, which is one big goal in ember modern apps.

The move to v2 addon supports like before FA5 & FA6.

Changes

  • Created a new ember addon by using Addon blueprint
  • Move FaIcon component to addon folder and old test-filed into test-app (we don't need anymore the rest)
  • Convert addon to TypeScript & glint
  • Change icon import logic like it is in angular or vue package of FA
  • Install fastboot package in test-app, so that we are save that it works also inside fastboot apps (this was never tested since now)
  • Readd support for ember v3.28 (because there is no blocking part for non supporting that version and requested in add back support for ember 3.28 #238)

Braking changes

  • change icon import
  • move @fortawesome/fontawesome-svg-core to peerDependencies
  • remove config option enableExperimentalBuildTimeTransform which was never documented
  • Remove option warnIfNoIconsIncluded as it was used only in build time code (index.js) which doesn't exists in v2 addons

Since now the icons were added as string inside config/icons.js. While build time we have search and the specified icons in fontawesome package and imported.
Example:

module.exports = function () {
  return {
    'free-solid-svg-icons': [
      'coffee',
      'magic',
      'trash-alt',
    ],
    'free-regular-svg-icons': 'all',
    'free-brands-svg-icons': 'all',
  };
};

After moving to v2 addon you can import the icons in any point of app (we should document by default in app.js/ts and explain that you can do it in every point of app).

Example of app.ts

import Application from '@ember/application';
import Resolver from 'ember-resolver';
import loadInitializers from 'ember-load-initializers';
import config from 'test-app/config/environment';
import { library, config as faConfig } from '@fortawesome/fontawesome-svg-core';
import '@fortawesome/fontawesome-svg-core/styles.css';
import {
  faCoffee,
  faMagic,
  faTrashAlt,
} from '@fortawesome/free-solid-svg-icons';
import * as freeRegularSvgIcons from '@fortawesome/free-regular-svg-icons';
import * as freeBrandSvgIcons from '@fortawesome/free-brands-svg-icons';

faConfig.autoAddCss = false;

library.add(
  faCoffee,
  faMagic,
  faTrashAlt,
);

library.add(freeBrandSvgIcons['fab']); // option to import all icons of fab
library.add(freeRegularSvgIcons['far']); // option to import all icons of far

export default class App extends Application {
  modulePrefix = config.modulePrefix;
  podModulePrefix = config.podModulePrefix;
  Resolver = Resolver;
}

loadInitializers(App, config.modulePrefix);

The app must import also the styles directly from font awesome svg package... So we give user the possibility to import the css in any place of app (its possible to add inside a component or in scss file...)

For fastboot apps we must teach to use config.autoAddCss = false otherwise the app isnt working.
In general we should recommend to import styles directly instead of auto DOM add, because it solve the issue that icons have wrong sizes since the css is loaded (the example should be show best practice use) and the behaviour is equal like today.

An additional reason to move to v2 addon solves a very long issue. Since now there was always necessary to stop and restart ember s when you have added icon, because we have created the file on first build time. With v2 addon you can add it, save changes, app makes a reload and icon is displayed.

Open ToDo's

if you like this concept, we can process by updating documentation (btw... in current docs... we speak often only from FA v5 😊) and minimal changes of console.warnings texts inside component

We can add release it for easier deploying... if you want

@@ -43,7 +42,6 @@ library.add(
faSync,
faStopwatch20,
faTrashAlt,
faCalendarXmark,
Copy link
Contributor Author

@mkszepp mkszepp Oct 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was missing in v2 branch so i did added because it was used in test-app... but in fa5 the icon doesn't exists so there was necessary to remove again

This was referenced Oct 27, 2024
@mkszepp
Copy link
Contributor Author

mkszepp commented Oct 30, 2024

@jrjohnson @robmadole can we get a review?

@jrjohnson
Copy link
Collaborator

It looks great, but I'm not an every day user of the addon anymore. I'll defer review to @robmadole and the FA team.

@mkszepp
Copy link
Contributor Author

mkszepp commented Nov 11, 2024

@robmadole can we get a review?

@robmadole
Copy link
Member

@mkszepp we'll put this on the todo list but it's probably going to be a bit.

@mkszepp
Copy link
Contributor Author

mkszepp commented Nov 12, 2024

@robmadole okay thanks for feedback... i will look to prepare the docs in next time and adding support for @glimmer/components v2 as it was landed some days ago, than this PR is completely ready for merge

@mkszepp
Copy link
Contributor Author

mkszepp commented Dec 1, 2024

@robmadole now all necessary changes / preparations were done

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 this pull request may close these issues.

3 participants