refactor(many): merge lucide icon package into ui-icons #2367
Conversation
joyenjoyer
force-pushed
the
icon_package_merge
branch
from
cfd8ac6 to
759845b
Compare
|
joyenjoyer
force-pushed
the
icon_package_merge
branch
3 times, most recently
from
59ef44b to
117ace8
Compare
joyenjoyer
force-pushed
the
icon_package_merge
branch
from
117ace8 to
ddc94f8
Compare
There was a problem hiding this comment.
I moved this utility function into ui-icons because it has specific, icon-related use cases. If someone imports ui-icons, these utilities will already be available, and there’s no use case where someone would need these utility functions without importing ui-icons.
packages/ui-icons/package.json
Outdated
| "build": "ui-scripts build __build__ --copy-files --modules es,cjs", | ||
| "build": "pnpm run build:main && pnpm run build:lucide", | ||
| "build:main": "ui-scripts build __build__ --copy-files --modules es,cjs", | ||
| "build:lucide": "pnpm exec babel lucide --out-dir lib/lucide --extensions .ts,.tsx,.js,.jsx && ES_MODULES=true pnpm exec babel lucide --out-dir es/lucide --extensions .ts,.tsx,.js,.jsx", |
There was a problem hiding this comment.
These special commands will be removed when old icons will be deleted.
packages/ui-icons/package.json
Outdated
| "exports": { | ||
| ".": { | ||
| "import": "./es/index.js", | ||
| "require": "./lib/index.js", | ||
| "types": "./types/index.d.ts" | ||
| }, | ||
| "./lucide": { | ||
| "import": "./es/lucide/index.js", | ||
| "require": "./lib/lucide/index.js", | ||
| "types": "./types/lucide/index.d.ts" | ||
| }, | ||
| "./lib/*": "./lib/*", | ||
| "./es/*": "./es/*", | ||
| "./types/*": "./types/*" | ||
| }, |
There was a problem hiding this comment.
This should not be needed if you export everything to the root of the package (its handed by the "module", "main" part above). While this is the "right" way to do it, we do it differently by convention in our other packages. When we change it we should change it everywhere
packages/ui-icons/package.json
Outdated
| ".": { | ||
| "import": "./es/index.js", | ||
| "require": "./lib/index.js", | ||
| "types": "./types/index.d.ts" | ||
| }, | ||
| "./lucide": { | ||
| "import": "./es/lucide/index.js", | ||
| "require": "./lib/lucide/index.js", | ||
| "types": "./types/lucide/index.d.ts" |
There was a problem hiding this comment.
The new icons should be exported from the root too for multiple reasons:
- We are always exporting everything from the root, we even have an ESLint rule against importing from anywhere else (but its not working 100% :( ).
- This would require a ton of renames from users (e.g.
import {IconXLine} from "@instructure/ui-icons"->import {IconXLine} from "@instructure/ui-icons/lucide") - There should be no name clashes with the old icons (and in the future they will point to a Lucide icon anyway)
packages/__docs__/globals.ts
Outdated
| const { renderIconWithProps, ...lucideIconComponents } = LucideIcons | ||
|
|
||
| const globals = { | ||
| ...Components, | ||
| ...lucideIconComponents, |
There was a problem hiding this comment.
AFAIK this is not needed if you export the new icons to the root, resolve.mjs should take care of this
joyenjoyer
force-pushed
the
icon_package_merge
branch
2 times, most recently
from
c45a2b6 to
b2190b7
Compare
matyasf
left a comment
matyasf
left a comment
There was a problem hiding this comment.
almost good, just a small change
joyenjoyer
force-pushed
the
icon_package_merge
branch
from
b2190b7 to
04e72a8
Compare
No description provided.