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

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

Closed
wants to merge 4 commits into from

Conversation

Scrum
Copy link
Contributor

@Scrum Scrum commented Jan 25, 2024

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.

Copy link

stackblitz bot commented Jan 25, 2024

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

Copy link

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 userquin self-requested a review January 25, 2024 10:46
@userquin
Copy link
Member

userquin commented Jan 25, 2024

@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
Copy link
Member

userquin commented Jan 25, 2024

@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
Copy link
Contributor Author

Scrum commented Jan 25, 2024

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

  • This is not the job of loadNodeIcon, hence it is part of this package.
  • This can be an array of values in order of priority where 0 is the highest priority
{
  scope: ["@my-primary-json", "@my-secondary-json" ,"@iconify-icon"]
}
  • What should we do if there is an intersection and we want something other than a higher priority package?
{
  scope: ["@my-primary-json", "@my-secondary-json" ,"@iconify-icon"]
}

loadNodeIcon('collection-name', 'icon-name', { scope: '@my-secondary-json' })

@Scrum
Copy link
Contributor Author

Scrum commented Jan 25, 2024

@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
Copy link
Member

userquin commented Jan 25, 2024

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
Copy link
Member

@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
Copy link
Contributor Author

Scrum commented Jan 25, 2024

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
Copy link
Member

userquin commented Jan 25, 2024

@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
Copy link
Member

@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
Copy link
Member

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

@userquin userquin closed this Jan 25, 2024
@Scrum
Copy link
Contributor Author

Scrum commented Jan 26, 2024

Solution for me

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

addCollection(icons);

@userquin
Copy link
Member

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

FYI: any @iconify icon component is client only.

@Scrum
Copy link
Contributor Author

Scrum commented Jan 26, 2024

@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
Copy link
Member

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: iconify/iconify#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
Copy link
Contributor Author

Scrum commented Jan 26, 2024

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
Copy link
Member

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

imagen

@Scrum
Copy link
Contributor Author

Scrum commented Jan 26, 2024

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
Copy link
Member

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

@Scrum
Copy link
Contributor Author

Scrum commented Jan 26, 2024

@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
Copy link
Member

userquin commented Jan 26, 2024

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
Copy link
Member

iconify/iconify#276

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.

2 participants