unplugin / unplugin-icons

🤹 Access thousands of icons as components on-demand universally.
https://www.npmjs.com/package/unplugin-icons
MIT License
4.12k stars 143 forks source link

perf(core/options) added scope of the icon sources package #341

Closed Scrum closed 10 months ago

Scrum commented 10 months ago

Description

This performance will make it possible to use automatic loading of icons in the package unplugin-icons from arbitrary packages.

Linked Issues

Additional context

I will be glad to receive any comments and suggestions.

stackblitz[bot] commented 10 months ago

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

socket-security[bot] commented 10 months ago

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@iconify/utils@2.1.17 Transitive: environment, filesystem, shell +29 1.13 MB

🚮 Removed packages: npm/@iconify/utils@2.1.12

View full report↗︎

userquin commented 10 months ago

@Scrum with this PR you can only use 1 source (scope) (?), we need to add new node loader (s) to allow use it inside the collection. How can we use multiple @iconify-icon/* collections with any other custom scope ones?

loader.ts module is using iconify loadNodeIcon in generateComponent.

userquin commented 10 months ago

@Scrum I'm going to refactor this PR to include a new loader for new scope removing the scope from global configuration or do you want to give it a try? We also need to add a test for scope, for example adding 2 scoped collections in examples/vite-vue3 (you have included a fixture in iconify IIRC, we can use it here => @test-scope folder).

EDIT: use file protocol in dev dependencies.

Scrum commented 10 months ago

Good point, I’m not sure if this is worth implementing in the current functionality, but my thoughts are:

{
  scope: ["@my-primary-json", "@my-secondary-json" ,"@iconify-icon"]
}

loadNodeIcon('collection-name', 'icon-name', { scope: '@my-secondary-json' })
Scrum commented 10 months ago

@Scrum I'm going to refactor this PR to include a new loader for new scope removing the scope from global configuration or do you want to give it a try? We also need to add a test for scope, for example adding 2 scoped collections in examples/vite-vue3 (you have included a fixture in iconify IIRC, we can use it here => @test-scope folder).

EDIT: use file protocol in dev dependencies.

Great, let me know if there is anything I can do or should do.

userquin commented 10 months ago

I'm going to review iconify PR, I think your PR there has introduced a breaking change. loadNodeIcon should have a new flag to avoid lookup icon in iconify collections (when enabled), otherwise we'll need to use loadIcon here if the collection scope isn't any iconify.

userquin commented 10 months ago

@Scrum we should add a lot of stuff here to resolve the scoped collections before calling loadNodeIcon, there is no correlation between the scope and the collections (for example, we should parse scoped package exports or scan scoped package folders). With your iconify PR you're adding multiple UniversalIconLoader, this repository is using only 1 UniversalIconLoader, that's loadNodeIcon.

If you check loadIcon in node-loader.ts module, it only checks for customCollections, then, loadNodeIcon will call loadCollectionFromFS if not found.

Scrum commented 10 months ago

It is still difficult for me to grasp the meaning of your direction.

I will describe how I would like to use this with examples:

npm package

@my-scope
 - icons
 -- package.json
 -- dist
 --- index.js 

vite.config.js

import IconsResolver from "unplugin-icons/resolver";
import Icons from "unplugin-icons/vite";
import Components from "unplugin-vue-components/vite";
import { defineConfig } from "vite";

export default defineConfig({
  plugins: [
    Components({
      resolvers: [IconsResolver({ componentPrefix: "my-collection", customCollections: ['icons'], })],
    }),
    Icons({
      autoInstall: true,
      scope: "@my-scope",
    }),
  ]
});

my-vue-component.vue

<my-collection-icons-user />

I understand that if I want to use another scope, I will no longer be able to do so.


As far as I understand, we need to be able to multiscoping, I suggested options earlier

vite.config.js

import IconsResolver from "unplugin-icons/resolver";
import Icons from "unplugin-icons/vite";
import Components from "unplugin-vue-components/vite";
import { defineConfig } from "vite";

export default defineConfig({
  plugins: [
    Components({
      resolvers: [IconsResolver({ componentPrefix: "my-collection", customCollections: ['icons'], })],
    }),
    Icons({
      autoInstall: true,
      scope: ["@my-scope", "my-super-scope"],
    }),
  ]
});

There's still the issue of race.


I don't think the multiscoping resolution should be in iconify, it should be in this package.


I tried my best to describe everything (thoughts/ideas and suggestions) in this post. Let me know if I'm heading in the wrong direction.

userquin commented 10 months ago

@Scrum the solution is not correct in iconify repo, since we need to know the scope before knowing what collection we need to resolve: the loadNodeIcon requires the scope, and so what scope should we use before knowing the collection (there is no correlation)? It is working with iconify-json since you don't change the logic using @iconify-json even providing another scope, how about using 2 scoped collections in your example? you can use only 1, check generateComponent.

EDIT: about there is no correlation => given a collection, what is the scope of that collection?

userquin commented 10 months ago

@Scrum this is my first try in iconify, we'll need to call loadCollectionFromFS in L23 to detect the scope (per icon) !. No makes sense, we need another solution (for example, a new custom loader)

imagen

userquin commented 10 months ago

@Scrum your iconify PR reverted in 2.1.19 after talking with @cyberalien

Scrum commented 10 months ago

Solution for me

import { addCollection } from "@iconify/vue";
import { icons } from "@my-scope/icons";

addCollection(icons);
userquin commented 10 months ago

@Scrum we need to find a better way to load external collections.

FYI: any @iconify icon component is client only.

Scrum commented 10 months ago

@Scrum we need to find a better way to load external collections.

FYI: any @iconify icon component is client only.

Undoubtedly, now I lack a complete picture of the code base of both projects and it will take me considerable time to dive in and find a solution. I will take into account all the points you provided in the comments above and try to find the best way.

Thanks anyway.

userquin commented 10 months ago

FYI: I moved utils/loader package from this repo to @iconfiy/utils package a few months ago to allow use the universal icon loader also in UnoCSS Preset Icons, you can check the integrations here:

and the iconify PR here: https://github.com/iconify/iconify/pull/97

In the meantime, you can use a custom loader to load your package, add the collection to the customCollections entry and a new loaded like this one: https://github.com/iconify/tools/blob/main/%40iconify-demo/unocss/unocss.config.ts#L16-L72

If you want I can help you with a GH repo to show you how to use it. Previous example in iconify tools requires some changes for your needs.

Scrum commented 10 months ago

In the meantime, you can use a custom loader to load your package, add the collection to the customCollections entry and a new loaded like this one: https://github.com/iconify/tools/blob/main/%40iconify-demo/unocss/unocss.config.ts#L16-L72

Thanks, I'll look into this.

If you want I can help you with a GH repo to show you how to use it. Previous example in iconify tools requires some changes for your needs.

I need time to immerse myself in projects to understand the needs. Now I only have a goal but no understanding.

userquin commented 10 months ago

@Scrum using custom loader and @test-scope in the iconify test fixtures, I'm going to publish the repo:

imagen

Scrum commented 10 months ago

using custom loader and @test-scope in the iconify test fixtures, I'm going to publish the repo:

Wow, that's cool, you're very fast 🚀

userquin commented 10 months ago

@Scrum https://github.com/userquin/custom-scoped-unplugin-icons-loader, check custom-loader.ts and the unplugin icons configuration.

Scrum commented 10 months ago

@Scrum https://github.com/userquin/custom-scoped-unplugin-icons-loader, check custom-loader.ts and the unplugin icons configuration.

This is incredibly cool, I can't keep up with you. I am incredibly grateful to you.

userquin commented 10 months ago

I'm going to send a new PR to iconify to include the new CustomScopedPackage function (maybe with another name), will be a mix of your original PR and my last PR to fix the fs name normalization to allow importModule work on Windows.

EDIT: this way we don't need to change anything here and in UnoCSS Preset Icons, just using ...CustomScopedPackage('<scoped-package-name>') inside customCollections option.

userquin commented 10 months ago

https://github.com/iconify/iconify/pull/276