Skip to content

Replace svelte-hero-icons and @zipline-ai/icons with unplugin-icons #133

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

Merged
merged 8 commits into from
Dec 20, 2024

Conversation

sean-zlai
Copy link
Contributor

@sean-zlai sean-zlai commented Dec 16, 2024

Summary

Replace svelte-hero-icons and @zipline-ai/icons with unplugin-icons which improve:

  • Simplifies using other icon sets (ex. Lucide)
  • Simplifies adding custom icons (see: vite.confg.ts
  • Is well maintained
    • unplugin-icons last released 2 days ago
    • svelte-hero-icons last released 5 months ago)
    • Used by many frameworks using Vite including Svelte, Vue, and React.
  • Installing the vscode-iconify plugin makes the experience 1000% better. Also browsing iconsets using icones.js.org which have quick copy/paste for "Unplugin Icons".
Before After
image image

Checklist

  • Added Unit Tests
  • Covered by existing CI
  • Integration tested
  • Documentation update

Summary by CodeRabbit

  • New Features

    • Introduced new icon packages for enhanced visual elements in the application.
  • Improvements

    • Updated multiple components to utilize new icon imports, streamlining icon rendering and improving maintainability.
  • Bug Fixes

    • Removed outdated icon dependencies to prevent potential conflicts and ensure compatibility with new imports.
  • Documentation

    • Added type exports from the new icon plugin for better type accessibility across the application.

Copy link

coderabbitai bot commented Dec 16, 2024

Walkthrough

This pull request introduces a comprehensive refactoring of icon management across the frontend project. The changes involve replacing the svelte-hero-icons library with a new icon management approach using unplugin-icons and @iconify-json/heroicons. Multiple components have been updated to use new icon imports from ~icons/heroicons, and the package.json has been modified to reflect these dependency changes. The modifications aim to streamline icon usage and improve the project's icon management strategy.

Changes

File Change Summary
frontend/package.json - Added @iconify-json/heroicons and unplugin-icons to devDependencies
- Removed svelte-hero-icons and @zipline-ai/icons
frontend/vite.config.ts Added unplugin-icons/vite plugin with Svelte compiler and custom icon collections
frontend/src/app.d.ts Added type export for unplugin-icons/types/svelte
frontend/src/lib/components/* Replaced svelte-hero-icons icon imports with ~icons/heroicons across multiple components
frontend/src/lib/types/Entity/Entity.ts Updated icon references in entityConfig
frontend/src/routes/joins/[slug]/+page.svelte Updated icon imports and usage

Sequence Diagram

sequenceDiagram
    participant Dev as Developer
    participant PackageJSON as package.json
    participant ViteConfig as vite.config.ts
    participant Components as Svelte Components

    Dev->>PackageJSON: Remove svelte-hero-icons
    Dev->>PackageJSON: Add unplugin-icons
    Dev->>ViteConfig: Configure Icons plugin
    Dev->>Components: Update icon imports
    Components->>Components: Replace Icon usage
Loading

Possibly related PRs

Suggested reviewers

  • ken-zlai

Poem

🐰 Hop, hop, icons take flight!
From hero to iconify, a delightful sight
Unplugged, replugged, with style so neat
Our frontend's icons now look so sweet!
Code refactored with rabbit-like grace 🎉


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between f2af557 and e32a91f.

📒 Files selected for processing (1)
  • frontend/src/app.d.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • frontend/src/app.d.ts

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@sean-zlai sean-zlai changed the title Setup unplugin-icons with @iconify-json/heroicons Setup unplugin-icons with @iconify-json/heroicons (WIP) Dec 16, 2024
@sean-zlai sean-zlai marked this pull request as draft December 16, 2024 22:45
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
frontend/package.json (1)

23-23: Consider documenting icon usage guidelines.

Since we're introducing a new icon system, it would be helpful to document:

  1. When to use @iconify-json/heroicons vs svelte-hero-icons
  2. How to import and use icons in components
  3. Best practices for icon usage

Would you like me to help create documentation for the icon system usage?

Also applies to: 54-54

frontend/src/lib/components/NavigationBar/NavigationBar.svelte (2)

38-39: Remove commented import and verify icon size consistency.

The commented import can be safely removed since we're now using the solid variant. Also, ensure that the 16px size is consistent with the design system, as the previous implementation used a dynamic size prop.

-// import IconMagnifyingGlass from '~icons/heroicons/magnifying-glass';
import IconMagnifyingGlass from '~icons/heroicons/magnifying-glass-16-solid';

38-39: LGTM! Clean implementation of unplugin-icons.

The changes successfully integrate unplugin-icons while maintaining existing functionality:

  1. Proper usage of the new icon system
  2. Preserved styling and behavior
  3. Consistent with the PR objectives

Consider documenting the icon usage pattern in the project's frontend guidelines to ensure consistency across components.

Also applies to: 105-106

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between a771452 and 73b5792.

⛔ Files ignored due to path filters (2)
  • frontend/package-lock.json is excluded by !**/package-lock.json
  • frontend/pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (4)
  • frontend/package.json (2 hunks)
  • frontend/src/lib/components/NavigationBar/NavigationBar.svelte (2 hunks)
  • frontend/tsconfig.json (1 hunks)
  • frontend/vite.config.ts (1 hunks)
🔇 Additional comments (5)
frontend/vite.config.ts (1)

3-3: LGTM! Plugin configuration looks correct.

The unplugin-icons integration is properly configured with the Svelte compiler option. The plugin placement after sveltekit() in the plugins array is appropriate since SvelteKit's plugin should generally be first.

Also applies to: 6-11

frontend/tsconfig.json (1)

12-15: LGTM! TypeScript configuration is properly set up.

The type definition for unplugin-icons is correctly configured. The moduleResolution is appropriately set to "bundler" which is compatible with the added types.

frontend/package.json (2)

54-54: Verify the unplugin-icons version for security.

Let's check if version 0.22.0 is the latest stable version and free from known vulnerabilities.

✅ Verification successful

The unplugin-icons version 0.22.0 is secure and up-to-date

The package is using the latest available version (0.22.0) and there are no known security vulnerabilities reported in the GitHub Security Advisory Database.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check latest version and security advisories
curl -s https://registry.npmjs.org/unplugin-icons/latest | jq '.version'

# Check for security advisories
gh api graphql -f query='
{
  securityVulnerabilities(first: 5, ecosystem: NPM, package: "unplugin-icons") {
    nodes {
      advisory {
        summary
        severity
        publishedAt
      }
      vulnerableVersionRange
      firstPatchedVersion {
        identifier
      }
    }
  }
}'

Length of output: 429


23-23: Verify compatibility with existing icon package.

I notice that svelte-hero-icons is already present in devDependencies. Consider if both packages are necessary or if one should be removed to avoid duplication.

✅ Verification successful

No duplication concern - packages serve different purposes

The added @iconify-json/heroicons package is a dependency of unplugin-icons and is used differently from svelte-hero-icons. From the codebase analysis:

  • svelte-hero-icons is used for component-based icons (e.g., import { Icon, Plus } from 'svelte-hero-icons')
  • @iconify-json/heroicons with unplugin-icons is used for direct icon imports (e.g., import IconMagnifyingGlass from '~icons/heroicons/magnifying-glass')

Both packages are correctly placed in devDependencies and serve complementary purposes in the project.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for potential icon component duplications
rg -l "heroicons" --type svelte

Length of output: 62


Script:

#!/bin/bash
# Check for icon-related imports and usage in all files
rg -l "heroicons|svelte-hero-icons"

# Check package.json for existing icon-related dependencies
cat frontend/package.json | jq '.dependencies,.devDependencies' | grep -i "icon"

Length of output: 1055


Script:

#!/bin/bash
# Check how these icon packages are being used in the codebase
rg "@zipline-ai/icons|svelte-hero-icons|heroicons" -B2 -A2

# Check if there are any imports from both packages in the same file
rg "import.*(@zipline-ai/icons|svelte-hero-icons|heroicons)" --type ts --type svelte

Length of output: 14269

frontend/src/lib/components/NavigationBar/NavigationBar.svelte (1)

105-106: Remove commented code and verify icon styling consistency.

The old implementation is commented out and can be safely removed. However, let's verify the styling consistency:

  1. The old implementation used micro prop and explicit size="16" - ensure the new 16px solid icon maintains visual consistency with other icons in the navigation bar.
  2. The text-foreground class is preserved, maintaining the color scheme.
✅ Verification successful

Remove commented code - icon styling is consistent

The commented code can be safely removed as the new implementation maintains consistency with the codebase's icon patterns:

  1. The new IconMagnifyingGlass is imported from ~icons/heroicons/magnifying-glass-16-solid, which follows the pattern of using 16px solid icons
  2. The text-foreground class is preserved, maintaining color consistency with other navigation elements
  3. Multiple instances of size="16" with micro prop are found throughout the codebase, and the new solid icon implementation inherently includes this sizing
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent icon usage patterns in the codebase

# Look for other icon size definitions to verify consistency
rg -A 1 'size="16"' --type svelte

# Check for other unplugin-icons usage patterns
rg "from '~icons/heroicons/.+-16-" --type svelte

Length of output: 151


Script:

#!/bin/bash
# Description: Check for consistent icon usage patterns in the codebase without file type filters

# Look for other icon size definitions to verify consistency
rg -A 1 'size="16"'

# Look for other IconMagnifyingGlass usages
rg "IconMagnifyingGlass"

# Look for other icon imports from heroicons
rg "from '~icons/heroicons"

# Look for other Icon component usages with MagnifyingGlass
rg "Icon.*MagnifyingGlass"

Length of output: 6625

@sean-zlai sean-zlai requested a review from ken-zlai December 18, 2024 04:41
@sean-zlai sean-zlai marked this pull request as ready for review December 18, 2024 04:41
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (6)
frontend/src/lib/components/BreadcrumbNav/BreadcrumbNav.svelte (1)

23-23: LGTM! Improved icon usage syntax

The new <IconSlash /> syntax is more concise and type-safe compared to the previous implementation. This change simplifies the code while maintaining the same functionality.

Consider adding a comment in the PR description mentioning that the vscode-iconify plugin provides autocompletion for these new icon imports, which could help other developers working with this new pattern.

frontend/src/lib/components/CollapsibleSection/CollapsibleSection.svelte (2)

45-45: LGTM! Consider organizing Tailwind classes

The icon implementation and animation logic work correctly. For better readability, consider organizing the Tailwind classes.

-<IconChevronDown class="transition-transform duration-200 {open ? '' : 'rotate-180'}" />
+<IconChevronDown class="transition-transform duration-200 {open ? 'rotate-0' : 'rotate-180'}" />

Line range hint 8-45: Icon system migration improves maintainability

The transition to unplugin-icons with @iconify-json/heroicons brings several benefits:

  1. Better maintainability through direct imports
  2. Improved type safety
  3. Simplified icon usage syntax
  4. Access to a wider range of icons through the Iconify ecosystem
frontend/src/lib/components/CustomEChartLegend/CustomEChartLegend.svelte (1)

123-125: Consider standardizing transition durations.

The implementation looks good! The icon rotation animation is well-implemented. However, I notice that the parent Button uses different transition durations for opacity (150ms) compared to the icon rotation (200ms).

Consider standardizing the duration for consistency:

-					class={cn('ml-2 transition-transform duration-200', isExpanded && '-rotate-180')}
+					class={cn('ml-2 transition-transform duration-150', isExpanded && '-rotate-180')}
frontend/src/lib/components/PageHeader/PageHeader.svelte (1)

20-21: Consider adding hover state styling.

The icon implementation looks good and maintains accessibility with the text label. However, you might want to consider matching the hover state color of the icon with the text for a more cohesive user experience.

-<IconBookOpen class="text-neutral-800" />
+<IconBookOpen class="text-neutral-800 group-hover:text-neutral-900" />
-<span class="text-neutral-800">Learn</span>
+<span class="text-neutral-800 group-hover:text-neutral-900">Learn</span>

Note: Add the group class to the Button component if not already present.

frontend/src/lib/types/Entity/Entity.ts (1)

1-21: Well-structured implementation of unplugin-icons.

The changes effectively transition from svelte-hero-icons to unplugin-icons while maintaining type safety and the existing entity configuration structure. The consistent naming convention and explicit size specifications in imports enhance code clarity.

Consider documenting the icon import pattern (e.g., in a README or contributing guide) to maintain consistency as more icons are added to the project.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 73b5792 and 3286bb2.

⛔ Files ignored due to path filters (3)
  • frontend/package-lock.json is excluded by !**/package-lock.json
  • frontend/src/lib/icons/chart-line.svg is excluded by !**/*.svg
  • frontend/src/lib/icons/chart-skew.svg is excluded by !**/*.svg
📒 Files selected for processing (14)
  • frontend/package.json (2 hunks)
  • frontend/src/lib/components/ActionButtons/ActionButtons.svelte (3 hunks)
  • frontend/src/lib/components/BreadcrumbNav/BreadcrumbNav.svelte (2 hunks)
  • frontend/src/lib/components/CollapsibleSection/CollapsibleSection.svelte (2 hunks)
  • frontend/src/lib/components/CustomEChartLegend/CustomEChartLegend.svelte (2 hunks)
  • frontend/src/lib/components/DateRangeSelector/DateRangeSelector.svelte (2 hunks)
  • frontend/src/lib/components/InfoTooltip/InfoTooltip.svelte (1 hunks)
  • frontend/src/lib/components/NavigationBar/NavigationBar.svelte (8 hunks)
  • frontend/src/lib/components/PageHeader/PageHeader.svelte (2 hunks)
  • frontend/src/lib/components/ResetZoomButton/ResetZoomButton.svelte (1 hunks)
  • frontend/src/lib/components/SplitView/SplitView.svelte (2 hunks)
  • frontend/src/lib/types/Entity/Entity.ts (1 hunks)
  • frontend/src/routes/joins/[slug]/+page.svelte (3 hunks)
  • frontend/vite.config.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • frontend/package.json
  • frontend/src/lib/components/NavigationBar/NavigationBar.svelte
🔇 Additional comments (22)
frontend/src/lib/components/BreadcrumbNav/BreadcrumbNav.svelte (1)

11-11: LGTM! Clean migration to unplugin-icons

The import statement has been successfully updated to use the new icon system, aligning with the PR's objective to replace svelte-hero-icons with unplugin-icons.

frontend/src/lib/components/ActionButtons/ActionButtons.svelte (2)

4-8: LGTM! Icon imports have been updated correctly.

The migration from svelte-hero-icons to direct imports from ~icons/heroicons follows the PR objectives of setting up unplugin-icons.


49-49: LGTM! Icon usage has been updated consistently.

The component template has been updated to use the new icon components directly, maintaining the same functionality while aligning with the new icon system.

Also applies to: 56-56, 65-65, 70-70, 75-75

frontend/src/routes/joins/[slug]/+page.svelte (2)

6-7: LGTM! Icon imports have been updated correctly.

The migration from svelte-hero-icons and @zipline-ai/icons to direct imports from ~icons/heroicons and ~icons/zipline-ai follows the PR objectives.


568-568: LGTM! Icon usage has been updated consistently.

The TabsTrigger components have been updated to use the new icon components directly, maintaining the same functionality while aligning with the new icon system.

Also applies to: 572-572

frontend/src/lib/components/SplitView/SplitView.svelte (2)

62-62: LGTM! Icon usage simplified.

The icon implementation has been simplified by directly using the IconXMark component while maintaining the same styling.


3-3: LGTM! Icon import updated correctly.

The import change aligns with the PR's objective to transition from svelte-hero-icons to unplugin-icons.

Let's verify the consistency of icon imports across the codebase:

✅ Verification successful

Icon imports have been consistently updated across the codebase

The verification shows:

  • No remaining imports from svelte-hero-icons were found
  • All heroicons are now consistently imported using the ~icons/heroicons/ pattern
  • The new imports follow a consistent naming convention with Icon prefix
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining svelte-hero-icons imports and verify the pattern of new imports

# Check for any remaining old imports
echo "Checking for remaining svelte-hero-icons imports:"
rg "from 'svelte-hero-icons'"

# Verify the pattern of new imports
echo "Verifying new icon import pattern:"
rg "from '~icons/heroicons/"

Length of output: 3240

frontend/src/lib/components/ResetZoomButton/ResetZoomButton.svelte (2)

10-10: LGTM! Cleaner icon implementation.

The direct usage of <IconArrowUturnLeft /> is more straightforward than the previous approach using <Icon> with props.


4-4: LGTM! Clean import using unplugin-icons.

The new import syntax is more direct and aligns with the PR's objective of transitioning to unplugin-icons.

Let's verify the icon path is correctly configured in Vite:

✅ Verification successful

Icon setup is correctly configured ✓

The verification confirms that:

  • unplugin-icons is properly set up in the Vite config
  • @iconify-json/heroicons is correctly installed as a dev dependency (v1.2.2)
  • The import path ~icons/heroicons follows the standard unplugin-icons convention
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if Vite config includes unplugin-icons setup
rg -l "unplugin-icons" frontend/vite.config*

# Verify the heroicons package is properly installed
cat frontend/package.json | jq '.dependencies["@iconify-json/heroicons"]'

Length of output: 151


Script:

#!/bin/bash
# Let's check the Vite config to ensure unplugin-icons is properly configured
rg -A 5 "unplugin-icons" frontend/vite.config.ts

# Check if heroicons might be in devDependencies instead
cat frontend/package.json | jq '.devDependencies["@iconify-json/heroicons"]'

# Check if there are any other icon-related packages that might be relevant
cat frontend/package.json | jq '.dependencies, .devDependencies' | rg -i "icon|hero"

Length of output: 434

frontend/src/lib/components/DateRangeSelector/DateRangeSelector.svelte (3)

13-13: LGTM: Import change aligns with the migration to unplugin-icons

The new import statement correctly uses the unplugin-icons convention and provides more specific icon metadata (size and style) in the import path.


134-134: LGTM: Simplified icon usage with unplugin-icons

The icon implementation is now more straightforward, eliminating the need for additional props while maintaining the same functionality.


13-13: Verify icon rendering and migration consistency

Let's ensure the icon migration pattern is consistent across the codebase and that the icon renders correctly.

Also applies to: 134-134

✅ Verification successful

Based on the script results, I can now generate the final response:

Icon migration pattern is consistent and properly implemented

The verification shows:

  • No remaining usage of svelte-hero-icons in the frontend
  • Consistent usage of unplugin-icons with the ~icons/heroicons/ pattern
  • The calendar icon follows the same size convention (16) and style (solid) as other icons in the codebase
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent icon migration patterns and potential issues

# Check for any remaining svelte-hero-icons usage
echo "Checking for remaining svelte-hero-icons usage..."
rg "from 'svelte-hero-icons'" frontend/

# Check for consistent unplugin-icons import pattern
echo "Checking for consistent unplugin-icons import pattern..."
rg "from '~icons/heroicons/" frontend/

# Check if the icon size (16) is consistently used for calendar icons
echo "Checking for consistent calendar icon usage..."
rg "calendar.*16.*solid" frontend/

Length of output: 3599

frontend/src/lib/components/CollapsibleSection/CollapsibleSection.svelte (1)

8-8: Verify the icon alias configuration in Vite

The import statement follows the unplugin-icons convention. Let's verify the configuration is properly set up.

✅ Verification successful

Icon configuration is properly set up

The verification confirms:

  • unplugin-icons is correctly configured in vite.config.ts with the Svelte compiler option
  • @iconify-json/heroicons is installed as a dev dependency (version ^1.2.2)
  • The import path ~icons/heroicons/chevron-down-16-solid follows the standard convention supported by the configuration
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the Vite configuration includes unplugin-icons setup

# Check if Vite config exists and contains unplugin-icons configuration
rg -l "unplugin-icons" frontend/vite.config.*

# Check if the specific heroicons package is listed in dependencies
cat frontend/package.json | jq '.dependencies["@iconify-json/heroicons"]'

Length of output: 151


Script:

#!/bin/bash
# Let's check the actual Vite configuration to ensure proper setup
cat frontend/vite.config.ts

# Also check devDependencies for the heroicons package
cat frontend/package.json | jq '.devDependencies["@iconify-json/heroicons"]'

Length of output: 947

frontend/src/lib/components/CustomEChartLegend/CustomEChartLegend.svelte (2)

4-5: LGTM! Clean import changes for icon management.

The changes align well with the PR objectives, replacing svelte-hero-icons with unplugin-icons. The addition of the cn utility suggests improved class name management.

Also applies to: 7-7


4-4: Verify type safety and consistent usage.

Let's ensure the new icon imports are properly typed and consistently used across the codebase.

✅ Verification successful

Icon import is properly typed and consistently used

The verification shows that:

  1. TypeScript is properly configured with "types": ["unplugin-icons/types/svelte"] in tsconfig.json
  2. All icon imports across the codebase consistently use the ~icons/heroicons pattern
  3. The CustomEChartLegend component is properly integrated within EChart.svelte and the icon import follows the same pattern as other components
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for consistent icon import patterns
echo "Checking for any remaining svelte-hero-icons imports..."
rg "from 'svelte-hero-icons'" frontend/

echo "Verifying consistent usage of unplugin-icons..."
rg "from '~icons/heroicons'" frontend/

echo "Checking TypeScript configuration..."
cat frontend/tsconfig.json | grep "types.*unplugin-icons"

Length of output: 463


Script:

#!/bin/bash
# Let's check if tsconfig.json exists and examine its content
echo "Checking tsconfig.json content:"
cat frontend/tsconfig.json

# Let's check for any icon imports in the codebase more broadly
echo -e "\nChecking all icon imports patterns:"
rg "import.*Icon.*from" frontend/

# Let's specifically look at the CustomEChartLegend component usage
echo -e "\nChecking CustomEChartLegend component usage:"
rg -A 5 "CustomEChartLegend" frontend/

Length of output: 5096

frontend/vite.config.ts (2)

3-4: LGTM! Imports are well-structured.

Good use of the node: prefix for the fs module import.


12-15: Verify custom icons setup and usage.

Let's ensure the custom icons are properly set up and used.

✅ Verification successful

Custom icons are properly set up and used

The verification confirms:

  • SVG files exist at the correct location: frontend/src/lib/icons/
  • Icons are being imported and used in the codebase (frontend/src/routes/joins/[slug]/+page.svelte)
  • The configuration in vite.config.ts correctly maps to the existing files
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify custom icons setup and usage

# Check if SVG files exist
echo "Checking SVG files existence..."
fd -t f "chart-(line|skew).svg" src/lib/icons

# Find usage of custom icons in the codebase
echo "Checking custom icons usage..."
rg -l "from ['\"]\~icons/zipline-ai/(chart-line|chart-skew)['\"]" -g "*.{svelte,ts,js}"

Length of output: 432


Script:

#!/bin/bash
# Let's verify the correct path for icons and their usage
echo "Checking repository structure for icons..."
fd -t f -e svg . frontend/src

echo "\nChecking the usage context..."
rg "from ['\"]~icons/zipline-ai/(chart-line|chart-skew)['\"]" -A 2 frontend/src/routes/joins/[slug]/+page.svelte

Length of output: 587

frontend/src/lib/components/PageHeader/PageHeader.svelte (1)

4-4: LGTM! Verify icon path availability.

The import statement correctly follows the unplugin-icons pattern.

Let's verify that the icon path exists in the heroicons set:

✅ Verification successful

Icon import is correctly implemented and widely used across the codebase

The verification shows that:

  • The book-open-16-solid icon is properly imported and used in PageHeader.svelte
  • The import pattern ~icons/heroicons/...-16-solid is consistently used across multiple components in the codebase
  • Other components successfully use the same unplugin-icons import pattern, confirming this is the established convention
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence of the book-open icon in the heroicons set
# Expected: The icon should be found in the installed package

# Check if the heroicons package is installed
if [ -d "node_modules/@iconify-json/heroicons" ]; then
    # Search for the book-open icon in the heroicons JSON files
    find node_modules/@iconify-json/heroicons -type f -name "*.json" -exec grep -l "book-open" {} \;
fi

Length of output: 49


Script:

#!/bin/bash
# Let's try a different approach to verify the icon's existence:

# 1. Check if the package.json has the required dependencies
echo "Checking package.json for heroicons dependencies:"
cat package.json | grep -i "heroicons"

# 2. Search for any files using this icon pattern
echo -e "\nSearching for files using the book-open icon:"
rg "book-open.*solid" --type svelte

# 3. Look for similar icon imports
echo -e "\nSearching for similar unplugin-icons imports:"
rg "import.*from '~icons/heroicons/" --type svelte

Length of output: 562


Script:

#!/bin/bash
# Let's try without file type restrictions since 'svelte' isn't recognized

# 1. Search for any files using this icon pattern
echo "Searching for files using the book-open icon:"
rg "book-open.*solid"

# 2. Look for similar unplugin-icons imports
echo -e "\nSearching for similar unplugin-icons imports:"
rg "import.*from '~icons/heroicons/"

# 3. Check if the file exists and show its content
echo -e "\nChecking the PageHeader.svelte content:"
cat frontend/src/lib/components/PageHeader/PageHeader.svelte

Length of output: 4265

frontend/src/lib/components/InfoTooltip/InfoTooltip.svelte (2)

3-3: Verify the Vite configuration for icon imports

The import path uses the ~icons alias which should be configured in the Vite setup.

✅ Verification successful

Import configuration is properly set up for unplugin-icons

The verification shows that:

  • The Vite configuration in frontend/vite.config.ts correctly includes the unplugin-icons/vite plugin with proper setup for Svelte compilation
  • TypeScript configuration includes the necessary types for unplugin-icons via "types": ["unplugin-icons/types/svelte"]
  • The comment in tsconfig.json indicates that path aliases are handled by SvelteKit's configuration, which is the standard way to handle the ~icons alias in a SvelteKit project
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify Vite configuration for unplugin-icons

# Check if Vite config includes the necessary plugin setup
rg -l "unplugin-icons/vite" 
rg -A 10 "unplugin-icons/vite" 

# Verify TypeScript configuration includes the type definitions
rg "~icons" tsconfig.json

Length of output: 727


Script:

#!/bin/bash
# Let's check for tsconfig files in the frontend directory and their content
fd tsconfig.json frontend/

# If found, check their content for path aliases
fd tsconfig.json frontend/ --exec cat {}

# Also check vite config for any path alias configurations
rg -A 5 "resolve:" frontend/vite.config.ts

Length of output: 839


10-10: LGTM! Consider documenting the icon styling system

The icon implementation is simplified and follows the new pattern correctly. The use of the text-muted-icon-neutral class suggests a systematic approach to icon styling.

✅ Verification successful

Icon styling pattern is consistent but could benefit from documentation

The text-muted-icon-neutral class is consistently used across the codebase, particularly in the NavigationBar and InfoTooltip components. While the icon implementation is correct and follows an established pattern, there's no explicit documentation of the icon styling system in the codebase. The pattern shows two main variants:

  • text-muted-icon-neutral for default state
  • text-muted-icon-primary for active state
  • Consider adding documentation about the icon styling system in the frontend's style guide or Tailwind configuration
  • Document the meaning and usage of text-muted-icon-neutral and text-muted-icon-primary classes
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent icon styling across the codebase

# Look for similar icon styling patterns
rg "text-muted-icon-" 

# Check if there's documentation about icon styling
fd -t f "README|CONTRIBUTING|STYLE" -e md | xargs rg -l "icon|styling"

Length of output: 489


Script:

#!/bin/bash
# Let's check for any Tailwind or CSS files that might define these icon styles
fd -e css -e pcss -e postcss | xargs rg "text-muted-icon"

# Also check for any style documentation in the app
fd . -t f | rg -i "style|theme|tailwind" | xargs rg -l "icon"

# Look for similar icon patterns to understand the styling system better
rg "Icon.*class=\"" -g "*.svelte"

Length of output: 1161

frontend/src/lib/types/Entity/Entity.ts (2)

9-9: LGTM! Icon component references updated correctly.

The icon property updates align with the new import names while maintaining the existing configuration structure.

Let's verify the icon components are properly exported from the unplugin-icons package:

Also applies to: 15-15, 21-21

✅ Verification successful

Icons are correctly imported and used consistently

The verification confirms that:

  • All required Heroicons (cube, puzzle-piece, square-3-stack-3d) are properly imported using the unplugin-icons syntax
  • The icons are consistently used across the codebase
  • No naming conflicts or missing exports were detected
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify icon component availability
# Check for potential icon naming conflicts or missing exports

rg "heroicons.*(?:cube|puzzle-piece|square-3-stack-3d)-16-solid"

Length of output: 531


1-3: LGTM! Verify icon sizes match the previous implementation.

The new imports correctly follow the unplugin-icons convention. The explicit 16px size specification in the import paths is noted.

Let's verify the previous icon sizes to ensure consistency:

✅ Verification successful

Let's gather more information to verify the icon sizes by checking the actual usage of these icons in the codebase.


Let's do one final check to see how these icons were previously imported and used before the migration:


Let's check the Vite and Tailwind configs since they might contain relevant icon configuration:


Based on all the gathered information, I can now generate the final response:

The icon size transition is correct and consistent

The migration from svelte-hero-icons to unplugin-icons maintains consistency by:

  1. Using the explicit 16px size variant (-16-solid) in the imports
  2. Proper configuration in vite.config.ts for the icon plugin
  3. Maintaining the same icon usage pattern in the entity configurations
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check previous icon size implementations
# Look for size-related properties in previous icon usage

rg -A 2 "svelte-hero-icons" 

Length of output: 618


Script:

#!/bin/bash
# Search for previous icon usage patterns in the codebase
rg -B 2 -A 2 "Cube|PuzzlePiece|Square3Stack3d" --type ts

# Also check for any size-related props or classes
ast-grep --pattern 'class $_ {
  $$$
  size = $_
  $$$
}'

Length of output: 1405


Script:

#!/bin/bash
# Check git history for previous implementation of Entity.ts
git show HEAD~1:frontend/src/lib/types/Entity/Entity.ts

# Also look for any icon-related configuration files
fd -e config.js -e config.ts -x grep -l "icon"

Length of output: 989


Script:

#!/bin/bash
# Check icon-related configurations
cat frontend/vite.config.ts
cat frontend/tailwind.config.js

# Also let's check for the previous commit that might have changed these icons
git log -p --grep="icon" -n 1

Length of output: 4533

Comment on lines 411 to 413
// TODO: Hacky workaround to keep page from locking up
return;

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical: Address the temporary workaround in loadDistributions.

The TODO comment indicates a temporary workaround that prevents the page from loading distributions. This could impact the functionality of the distributions tab.

Would you like me to help investigate the performance issue and propose a solution? I can help:

  1. Profile the distribution loading logic
  2. Implement pagination or virtualization
  3. Optimize the data fetching strategy

Comment on lines +9 to +17
Icons({
compiler: 'svelte',
customCollections: {
'zipline-ai': {
'chart-line': () => fs.readFile('./src/lib/icons/chart-line.svg', 'utf-8'),
'chart-skew': () => fs.readFile('./src/lib/icons/chart-skew.svg', 'utf-8')
}
}
})
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance robustness of custom icon loading.

While the plugin configuration is functional, consider these improvements:

  1. Use absolute paths to prevent working directory issues
  2. Add error handling for file operations
  3. Consider caching SVG content to improve performance

Here's the suggested implementation:

 Icons({
   compiler: 'svelte',
   customCollections: {
     'zipline-ai': {
-      'chart-line': () => fs.readFile('./src/lib/icons/chart-line.svg', 'utf-8'),
-      'chart-skew': () => fs.readFile('./src/lib/icons/chart-skew.svg', 'utf-8')
+      'chart-line': async () => {
+        try {
+          return await fs.readFile(
+            new URL('./src/lib/icons/chart-line.svg', import.meta.url),
+            'utf-8'
+          );
+        } catch (error) {
+          console.error('Failed to load chart-line icon:', error);
+          return ''; // Return empty SVG or a fallback
+        }
+      },
+      'chart-skew': async () => {
+        try {
+          return await fs.readFile(
+            new URL('./src/lib/icons/chart-skew.svg', import.meta.url),
+            'utf-8'
+          );
+        } catch (error) {
+          console.error('Failed to load chart-skew icon:', error);
+          return ''; // Return empty SVG or a fallback
+        }
+      }
     }
   }
 })
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Icons({
compiler: 'svelte',
customCollections: {
'zipline-ai': {
'chart-line': () => fs.readFile('./src/lib/icons/chart-line.svg', 'utf-8'),
'chart-skew': () => fs.readFile('./src/lib/icons/chart-skew.svg', 'utf-8')
}
}
})
Icons({
compiler: 'svelte',
customCollections: {
'zipline-ai': {
'chart-line': async () => {
try {
return await fs.readFile(
new URL('./src/lib/icons/chart-line.svg', import.meta.url),
'utf-8'
);
} catch (error) {
console.error('Failed to load chart-line icon:', error);
return ''; // Return empty SVG or a fallback
}
},
'chart-skew': async () => {
try {
return await fs.readFile(
new URL('./src/lib/icons/chart-skew.svg', import.meta.url),
'utf-8'
);
} catch (error) {
console.error('Failed to load chart-skew icon:', error);
return ''; // Return empty SVG or a fallback
}
}
}
}
})

@sean-zlai sean-zlai changed the title Setup unplugin-icons with @iconify-json/heroicons (WIP) Replace svelte-hero-icons and @zipline-ai/icons with unplugin-icons Dec 18, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🔭 Outside diff range comments (1)
frontend/src/lib/components/NavigationBar/NavigationBar.svelte (1)

Line range hint 205-224: Fix incorrect icon reference and optimize icon declarations.

Two issues need attention:

  1. The joins section is using the wrong icon (models instead of joins)
  2. The icon declarations can be optimized

Fix the incorrect icon reference:

-        {@const IconJoins = getEntity('models').icon}
+        {@const IconJoins = getEntity('joins').icon}

Consider moving the icon declarations outside the loop for better performance:

{#if searchResults.length > 0}
  {@const IconModels = getEntity('models').icon}
  {@const IconJoins = getEntity('joins').icon}
  {@const IconGroupBys = getEntity('groupbys').icon}
  <CommandGroup heading={`Search for "${input}"`}>
    {#each searchResults as model}
      <!-- Use the predeclared icon components -->
    {/each}
  </CommandGroup>
{/if}
🧹 Nitpick comments (1)
frontend/src/lib/components/NavigationBar/NavigationBar.svelte (1)

178-192: Consider adding visual feedback for disabled items.

While the icon implementation is correct, consider adding a CSS class to visually indicate the disabled state of these menu items (e.g., reducing opacity).

-						<IconExclamationTriangle />
+						<IconExclamationTriangle class="opacity-50" />
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between f8291f2 and fc7d0a0.

⛔ Files ignored due to path filters (4)
  • frontend/package-lock.json is excluded by !**/package-lock.json
  • frontend/pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
  • frontend/src/lib/icons/chart-line.svg is excluded by !**/*.svg
  • frontend/src/lib/icons/chart-skew.svg is excluded by !**/*.svg
📒 Files selected for processing (15)
  • frontend/package.json (2 hunks)
  • frontend/src/lib/components/ActionButtons/ActionButtons.svelte (3 hunks)
  • frontend/src/lib/components/BreadcrumbNav/BreadcrumbNav.svelte (2 hunks)
  • frontend/src/lib/components/CollapsibleSection/CollapsibleSection.svelte (2 hunks)
  • frontend/src/lib/components/CustomEChartLegend/CustomEChartLegend.svelte (2 hunks)
  • frontend/src/lib/components/DateRangeSelector/DateRangeSelector.svelte (2 hunks)
  • frontend/src/lib/components/InfoTooltip/InfoTooltip.svelte (1 hunks)
  • frontend/src/lib/components/NavigationBar/NavigationBar.svelte (8 hunks)
  • frontend/src/lib/components/PageHeader/PageHeader.svelte (2 hunks)
  • frontend/src/lib/components/ResetZoomButton/ResetZoomButton.svelte (1 hunks)
  • frontend/src/lib/components/SplitView/SplitView.svelte (2 hunks)
  • frontend/src/lib/types/Entity/Entity.ts (1 hunks)
  • frontend/src/routes/joins/[slug]/+page.svelte (2 hunks)
  • frontend/tsconfig.json (1 hunks)
  • frontend/vite.config.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
  • frontend/tsconfig.json
  • frontend/src/lib/components/DateRangeSelector/DateRangeSelector.svelte
  • frontend/src/lib/components/CollapsibleSection/CollapsibleSection.svelte
  • frontend/src/lib/components/CustomEChartLegend/CustomEChartLegend.svelte
  • frontend/src/lib/types/Entity/Entity.ts
  • frontend/src/lib/components/InfoTooltip/InfoTooltip.svelte
  • frontend/src/lib/components/PageHeader/PageHeader.svelte
  • frontend/src/lib/components/BreadcrumbNav/BreadcrumbNav.svelte
  • frontend/src/lib/components/SplitView/SplitView.svelte
  • frontend/package.json
  • frontend/src/routes/joins/[slug]/+page.svelte
  • frontend/src/lib/components/ActionButtons/ActionButtons.svelte
  • frontend/src/lib/components/ResetZoomButton/ResetZoomButton.svelte
  • frontend/vite.config.ts
🔇 Additional comments (5)
frontend/src/lib/components/NavigationBar/NavigationBar.svelte (5)

28-34: LGTM! Icon imports are well-organized.

The new imports using unplugin-icons are properly structured with consistent naming and clear size/style specifications.


100-100: LGTM! Icon usage is simplified.

The new icon component usage is more concise while maintaining the styling.


123-123: LGTM! Consistent icon implementation.

The docs button icon follows the same simplified pattern.


151-155: LGTM! Avatar section icons are properly updated.

Both the user icon and chevron icon implementations follow the new pattern consistently.


28-34: Verify icon configuration in Vite.

Let's ensure the icon plugin is properly configured in the Vite config.

✅ Verification successful

Icon configuration is properly set up and icons are correctly used

The verification shows that:

  • The Vite configuration has unplugin-icons properly configured with the svelte compiler
  • All imported heroicons are actively used in the NavigationBar component
  • The icon imports follow the correct ~icons/heroicons syntax
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check Vite configuration for unplugin-icons
rg -l 'unplugin-icons/vite' vite.config.ts
# Verify all icon imports are working
rg -l '~icons/heroicons' 'frontend/src/**/*.svelte'

Length of output: 216


Script:

#!/bin/bash
# Let's first find the vite config file and icon-related files
fd -t f "vite.config"
fd -t f "NavigationBar.svelte"

# Then check the content of these files
rg -l "unplugin-icons"
rg "import.*heroicons" "frontend/src/lib/components/NavigationBar/NavigationBar.svelte"

Length of output: 883


Script:

#!/bin/bash
# Let's check the vite.config.ts content to verify the icon plugin configuration
cat frontend/vite.config.ts

# Also check if these icons are actually used in the component
rg "Icon(ArrowsUpDown|AdjustmentsHorizontal|ChevronDown|DocumentText|ExclamationTriangle|MagnifyingGlass|User)" frontend/src/lib/components/NavigationBar/NavigationBar.svelte

Length of output: 1836

@sean-zlai sean-zlai force-pushed the sean/unplugin-icons branch 2 times, most recently from 10511b2 to 121a179 Compare December 19, 2024 22:44
@ken-zlai
Copy link
Contributor

Something funky is going on here. Pulling the latest from this branch, npm install, then npm run check gives me 14 errors. These errors don't exist in main.

Looking at the diff, I don't see why the errors are here (related to echarts imports, etc). Could it be some weird rebase thing?

kenmorton@Kens-MacBook-Pro frontend % npm run check

> [email protected] check
> svelte-kit sync && svelte-check --tsconfig ./tsconfig.json


====================================
Loading svelte-check in workspace: /Users/kenmorton/Documents/Git Repos/chronon/frontend
Getting Svelte diagnostics...

/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/util/chart-options.svelte.ts:1:15
Error: '"echarts"' has no exported member named 'EChartOption'. Did you mean 'EChartsOption'? 
import type { EChartOption } from 'echarts';
import merge from 'lodash/merge';


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/util/chart-options.svelte.ts:115:37
Error: Parameter 's' implicitly has an 'any' type. 
                const series = Array.isArray(customOption.series) ? customOption.series : [customOption.series];
                customOption.series = series.map((s) => merge({}, baseSeriesStyle, s));
        }


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/util/chart.ts:31:37
Error: Property 'findIndex' does not exist on type '{}'. 
        // Find the series index by name
        const seriesIndex = options.series.findIndex((s) => s.name === seriesName);
        if (seriesIndex === -1) return '#000000';


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/util/chart.ts:31:48
Error: Parameter 's' implicitly has an 'any' type. 
        // Find the series index by name
        const seriesIndex = options.series.findIndex((s) => s.name === seriesName);
        if (seriesIndex === -1) return '#000000';


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/components/EChart/EChart.svelte:4:32
Error: '"echarts"' has no exported member named 'EChartOption'. Did you mean 'EChartsOption'? (ts)
        import * as echarts from 'echarts';
        import type { ECElementEvent, EChartOption } from 'echarts';
        import merge from 'lodash/merge';


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/components/EChart/EChart.svelte:113:8
Error: Parameter 'point' implicitly has an 'any' type. (ts)
                                        const dataIndex = firstSeries.data.findIndex(
                                                (point) => (point as [number, number])[0] === exactX
                                        );


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/components/EChart/EChart.svelte:300:50
Error: Parameter 'prev' implicitly has an 'any' type. (ts)

                        const nearestPoint = firstSeries.data.reduce((prev, curr) => {
                                const [prevX] = prev as [number, number];


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/components/EChart/EChart.svelte:300:56
Error: Parameter 'curr' implicitly has an 'any' type. (ts)

                        const nearestPoint = firstSeries.data.reduce((prev, curr) => {
                                const [prevX] = prev as [number, number];


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/components/EChart/EChart.svelte:313:38
Error: Parameter 'point' implicitly has an 'any' type. (ts)

                                        const exactPoint = s.data.find((point) => (point as [number, number])[0] === exactX);
                                        if (!exactPoint) return null;


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/components/PercentileChart/PercentileChart.svelte:3:16
Error: '"echarts"' has no exported member named 'EChartOption'. Did you mean 'EChartsOption'? (ts)
        import EChart from '$lib/components/EChart/EChart.svelte';
        import type { EChartOption, EChartsType } from 'echarts';
        import type { TimeSeriesItem } from '$lib/types/Model/Model';


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/routes/joins/[slug]/+page.svelte:3:16
Error: '"echarts"' has no exported member named 'EChartOption'. Did you mean 'EChartsOption'? (ts)
        import EChart from '$lib/components/EChart/EChart.svelte';
        import type { EChartOption, EChartsType, ECElementEvent } from 'echarts';



/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/routes/joins/[slug]/+page.svelte:129:56
Error: Parameter 'item' implicitly has an 'any' type. (ts)
                                if (Array.isArray(series.data)) {
                                        const matchingPointIndex = series.data.findIndex((item) => {
                                                const [itemTimestamp, itemValue] = item as [string, number];


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/routes/joins/[slug]/+page.svelte:470:53
Error: Parameter 'point' implicitly has an 'any' type. (ts)
                                                // Find the point at the same timestamp
                                                const updatedPoint = updatedSeries.data.find((point) => {
                                                        const [pointTimestamp] = point as [number, number];


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/routes/observability/+page.svelte:6:32
Error: '"echarts"' has no exported member named 'EChartOption'. Did you mean 'EChartsOption'? (ts)
        import { generateXAxis, generateYAxis, generateHeatmapData } from '$lib/util/heatmap-data-gen';
        import type { ECElementEvent, EChartOption, EChartsType } from 'echarts';
        import { onMount } from 'svelte';


====================================
svelte-check found 14 errors and 0 warnings in 6 files

…ons.types` (which overrides defaults) to `app.d.ts`. Fixes ECharts types (svelte-check)
@sean-zlai
Copy link
Contributor Author

sean-zlai commented Dec 20, 2024

Something funky is going on here. Pulling the latest from this branch, npm install, then npm run check gives me 14 errors. These errors don't exist in main.

Looking at the diff, I don't see why the errors are here (related to echarts imports, etc). Could it be some weird rebase thing?

kenmorton@Kens-MacBook-Pro frontend % npm run check

> [email protected] check
> svelte-kit sync && svelte-check --tsconfig ./tsconfig.json


====================================
Loading svelte-check in workspace: /Users/kenmorton/Documents/Git Repos/chronon/frontend
Getting Svelte diagnostics...

/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/util/chart-options.svelte.ts:1:15
Error: '"echarts"' has no exported member named 'EChartOption'. Did you mean 'EChartsOption'? 
import type { EChartOption } from 'echarts';
import merge from 'lodash/merge';


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/util/chart-options.svelte.ts:115:37
Error: Parameter 's' implicitly has an 'any' type. 
                const series = Array.isArray(customOption.series) ? customOption.series : [customOption.series];
                customOption.series = series.map((s) => merge({}, baseSeriesStyle, s));
        }


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/util/chart.ts:31:37
Error: Property 'findIndex' does not exist on type '{}'. 
        // Find the series index by name
        const seriesIndex = options.series.findIndex((s) => s.name === seriesName);
        if (seriesIndex === -1) return '#000000';


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/util/chart.ts:31:48
Error: Parameter 's' implicitly has an 'any' type. 
        // Find the series index by name
        const seriesIndex = options.series.findIndex((s) => s.name === seriesName);
        if (seriesIndex === -1) return '#000000';


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/components/EChart/EChart.svelte:4:32
Error: '"echarts"' has no exported member named 'EChartOption'. Did you mean 'EChartsOption'? (ts)
        import * as echarts from 'echarts';
        import type { ECElementEvent, EChartOption } from 'echarts';
        import merge from 'lodash/merge';


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/components/EChart/EChart.svelte:113:8
Error: Parameter 'point' implicitly has an 'any' type. (ts)
                                        const dataIndex = firstSeries.data.findIndex(
                                                (point) => (point as [number, number])[0] === exactX
                                        );


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/components/EChart/EChart.svelte:300:50
Error: Parameter 'prev' implicitly has an 'any' type. (ts)

                        const nearestPoint = firstSeries.data.reduce((prev, curr) => {
                                const [prevX] = prev as [number, number];


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/components/EChart/EChart.svelte:300:56
Error: Parameter 'curr' implicitly has an 'any' type. (ts)

                        const nearestPoint = firstSeries.data.reduce((prev, curr) => {
                                const [prevX] = prev as [number, number];


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/components/EChart/EChart.svelte:313:38
Error: Parameter 'point' implicitly has an 'any' type. (ts)

                                        const exactPoint = s.data.find((point) => (point as [number, number])[0] === exactX);
                                        if (!exactPoint) return null;


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/lib/components/PercentileChart/PercentileChart.svelte:3:16
Error: '"echarts"' has no exported member named 'EChartOption'. Did you mean 'EChartsOption'? (ts)
        import EChart from '$lib/components/EChart/EChart.svelte';
        import type { EChartOption, EChartsType } from 'echarts';
        import type { TimeSeriesItem } from '$lib/types/Model/Model';


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/routes/joins/[slug]/+page.svelte:3:16
Error: '"echarts"' has no exported member named 'EChartOption'. Did you mean 'EChartsOption'? (ts)
        import EChart from '$lib/components/EChart/EChart.svelte';
        import type { EChartOption, EChartsType, ECElementEvent } from 'echarts';



/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/routes/joins/[slug]/+page.svelte:129:56
Error: Parameter 'item' implicitly has an 'any' type. (ts)
                                if (Array.isArray(series.data)) {
                                        const matchingPointIndex = series.data.findIndex((item) => {
                                                const [itemTimestamp, itemValue] = item as [string, number];


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/routes/joins/[slug]/+page.svelte:470:53
Error: Parameter 'point' implicitly has an 'any' type. (ts)
                                                // Find the point at the same timestamp
                                                const updatedPoint = updatedSeries.data.find((point) => {
                                                        const [pointTimestamp] = point as [number, number];


/Users/kenmorton/Documents/Git Repos/chronon/frontend/src/routes/observability/+page.svelte:6:32
Error: '"echarts"' has no exported member named 'EChartOption'. Did you mean 'EChartsOption'? (ts)
        import { generateXAxis, generateYAxis, generateHeatmapData } from '$lib/util/heatmap-data-gen';
        import type { ECElementEvent, EChartOption, EChartsType } from 'echarts';
        import { onMount } from 'svelte';


====================================
svelte-check found 14 errors and 0 warnings in 6 files

@ken-zlai - Fixed via e32a91f. Was caused because setting compilerOptions.types in tsconfig.json sets an explicit list of types to use (ignoring others in node_modules/@types, etc). We could add echarts to this list (which did fix the issue), but I found a better approach by re-exporting the types from app.d.ts.

PR is ready for a final review

image

Copy link
Contributor

@ken-zlai ken-zlai left a comment

Choose a reason for hiding this comment

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

I like how unplugin-icons is more future proof in terms of switching out icon packs.

Since we are now gonna store our icons in this repo, does it make sense to delete https://github.com/zipline-ai/icons as to avoid any confusion?

@sean-zlai
Copy link
Contributor Author

I like how unplugin-icons is more future proof in terms of switching out icon packs.

Since we are now gonna store our icons in this repo, does it make sense to delete https://github.com/zipline-ai/icons as to avoid any confusion?

Possibly, or mark the repo as archived?

@sean-zlai sean-zlai merged commit 8cc7a09 into main Dec 20, 2024
1 of 2 checks passed
@sean-zlai sean-zlai deleted the sean/unplugin-icons branch December 20, 2024 20:46
@coderabbitai coderabbitai bot mentioned this pull request Jan 20, 2025
4 tasks
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