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

chore: Update solidstart example #985

Merged
merged 7 commits into from
Sep 24, 2024
Merged

chore: Update solidstart example #985

merged 7 commits into from
Sep 24, 2024

Conversation

juraj98
Copy link
Contributor

@juraj98 juraj98 commented Sep 24, 2024

When working on example for tanstack/start I ran into CI lint issue because of different versions of vinxi for SolidStart and tanstack/start. I decided to update the solidstart example first. Full list of changes:

  • Updated dependencies to the newest version with appropriate code changes.
  • Removed tailwind and imported @uploadthing/solid/styles.css instead.
  • Removed unimportant meta tags
  • Changed few console.logs and alerts in index route to be consistent.

I'll update solid start getting started docs next and link the PR here.

@juliusmarminge, this might conflict with #980.

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced a new Tailwind CSS configuration for improved styling.
    • Added autoprefixer for better CSS compatibility.
  • Bug Fixes

    • Updated several dependencies, including solid-js, for enhanced performance and stability.
  • Documentation

    • Updated project configuration files to reflect new module syntax and engine requirements.
  • Chores

    • Removed outdated components and imports from the main application file.

Copy link

changeset-bot bot commented Sep 24, 2024

⚠️ No Changeset found

Latest commit: c4e381c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Sep 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs-uploadthing ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 24, 2024 4:13pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
legacy-docs-uploadthing ⬜️ Ignored (Inspect) Visit Preview Sep 24, 2024 4:13pm

Copy link
Contributor

coderabbitai bot commented Sep 24, 2024

Walkthrough

The pull request introduces updates to the @example/minimal-solidstart project, including modifications to the package.json file, changes to the PostCSS configuration, and alterations in the App component. Key updates involve the addition of the autoprefixer plugin, the introduction of a new Tailwind CSS configuration file, and the removal of certain imports. Additionally, several dependencies have been updated to their latest versions, and a minimum Node.js version requirement has been specified.

Changes

File Path Change Summary
examples/minimal-solidstart/package.json Updated package.json with repositioned fields, modified scripts, updated dependencies, and added Node.js engine requirement.
examples/minimal-solidstart/postcss.config.js Changed export style to ES module syntax and added autoprefixer plugin.
examples/minimal-solidstart/src/app.tsx Removed Title component, updated import statements for styles and routing.
examples/minimal-solidstart/tailwind.config.js Introduced new Tailwind CSS configuration specifying content sources and initialized theme and plugins.

Possibly related PRs

  • fix(docs): force path resolution #953: This PR modifies the Tailwind CSS configuration to enhance path resolution, which is relevant since the main PR introduces a new Tailwind CSS configuration file (tailwind.config.js) and includes the autoprefixer plugin.

Suggested reviewers

  • juliusmarminge
  • markflorkowski

🐰 In the meadow, changes bloom,
A project grows, dispelling gloom.
With uploads clear and styles anew,
The code hops forward, fresh as dew.
Let's celebrate, with joy and cheer,
For every change brings us near! 🌼


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
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 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.

Copy link
Contributor

@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: 1

Outside diff range and nitpick comments (5)
examples/minimal-solidstart/src/routes/api/uploadthing.ts (1)

11-12: LGTM: Explicit GET and POST handlers.

The new export statements for GET and POST handlers are a good improvement. They provide better clarity and adhere to SolidStart conventions. The use of the APIEvent type ensures type safety, and the handlers correctly pass the request object to the handler function.

For consistency with the import statement, consider using a type annotation for the return value. You can modify the lines as follows:

-export const GET = (event: APIEvent) => handler(event.request);
-export const POST = (event: APIEvent) => handler(event.request);
+export const GET = (event: APIEvent): Promise<Response> => handler(event.request);
+export const POST = (event: APIEvent): Promise<Response> => handler(event.request);

This change makes the return type explicit and consistent with the typical return type of API route handlers in SolidStart.

examples/minimal-solidstart/package.json (1)

21-23: LGTM: Node.js engine requirement added.

The addition of the "engines" field specifying a minimum Node.js version of >=18 is a good practice. It ensures that the project is run on a compatible Node.js version.

Suggestion: Consider adding this Node.js version requirement to the project's README or documentation to make it clear to other developers.

examples/minimal-solidstart/src/routes/index.tsx (3)

28-33: LGTM: Enhanced upload event handling for UploadButton

The addition of onUploadBegin and onUploadAborted handlers to the UploadButton component improves the upload process feedback. These changes align well with the PR objectives and provide more detailed information for both debugging and user experience.

Consider using a more user-friendly notification method instead of alert() for production code. For example, you could use a toast notification library or a custom modal component to provide a better user experience.


44-49: LGTM: Consistent event handling for UploadDropzone

The addition of onUploadBegin and onUploadAborted handlers to the UploadDropzone component maintains consistency with the UploadButton component and improves the overall upload process feedback. These changes align well with the PR objectives.

To reduce code duplication, consider extracting these common event handlers into separate functions or a custom hook. This would make the code more DRY and easier to maintain. For example:

const useUploadHandlers = () => {
  return {
    onUploadBegin: (fileName: string) => {
      console.log("onUploadBegin", fileName);
    },
    onUploadAborted: () => {
      alert("Upload Aborted");
    },
    onClientUploadComplete: (res: any) => {
      console.log(`onClientUploadComplete`, res);
      alert("Upload Completed");
    },
  };
};

// In your component:
const uploadHandlers = useUploadHandlers();

// Then use it like this:
<UploadButton
  endpoint="videoAndImage"
  {...uploadHandlers}
/>

<UploadDropzone
  endpoint="videoAndImage"
  {...uploadHandlers}
/>

This approach would centralize the event handling logic and make it easier to update in the future.


Line range hint 1-70: Overall assessment: Improved upload handling with room for refinement

The changes in this file successfully enhance the upload process feedback and logging, aligning well with the PR objectives. The consistent implementation across different upload components (UploadButton and UploadDropzone) is commendable.

However, there are opportunities for further improvement:

  1. Consider using a more user-friendly notification method instead of alert() for production code.
  2. To reduce code duplication, the common event handlers could be extracted into separate functions or a custom hook.

These refinements would improve code maintainability and user experience without changing the core functionality introduced in this PR.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 073c4ec and 047c4b2.

Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
Files selected for processing (12)
  • examples/minimal-solidstart/package.json (1 hunks)
  • examples/minimal-solidstart/postcss.config.cjs (0 hunks)
  • examples/minimal-solidstart/src/app.tsx (1 hunks)
  • examples/minimal-solidstart/src/entry-client.tsx (1 hunks)
  • examples/minimal-solidstart/src/entry-server.tsx (1 hunks)
  • examples/minimal-solidstart/src/global.css (0 hunks)
  • examples/minimal-solidstart/src/routes/api/uploadthing.ts (1 hunks)
  • examples/minimal-solidstart/src/routes/index.tsx (2 hunks)
  • examples/minimal-solidstart/src/server/uploadthing.ts (1 hunks)
  • examples/minimal-solidstart/src/utils/uploadthing.ts (2 hunks)
  • examples/minimal-solidstart/tailwind.config.ts (0 hunks)
  • examples/minimal-solidstart/tsconfig.json (1 hunks)
Files not reviewed due to no reviewable changes (3)
  • examples/minimal-solidstart/postcss.config.cjs
  • examples/minimal-solidstart/src/global.css
  • examples/minimal-solidstart/tailwind.config.ts
Files skipped from review due to trivial changes (1)
  • examples/minimal-solidstart/src/server/uploadthing.ts
Additional comments not posted (22)
examples/minimal-solidstart/src/entry-client.tsx (1)

1-1: Approved: Enhanced development experience with hot reloading

The addition of the // @refresh reload comment directive is a positive change that improves the development experience. This directive enables hot reloading, allowing the application to automatically refresh when changes are made during development. This aligns well with the PR's objective of updating the SolidStart example and enhancing developer productivity.

examples/minimal-solidstart/src/routes/api/uploadthing.ts (2)

1-1: LGTM: Import statement for APIEvent type.

The addition of the APIEvent type import from "@solidjs/start/server" is appropriate and necessary for the new handler functions. This change aligns well with the updates made to the export statements.


1-12: Overall, good improvements to API route handling.

The changes made to this file enhance the structure and type safety of the API route handlers for uploadthing in the SolidStart application. By explicitly defining separate GET and POST handlers, the code now adheres more closely to SolidStart conventions and provides better clarity.

These modifications will make the code more maintainable and easier to extend in the future if method-specific logic needs to be added. The use of TypeScript types throughout ensures good type safety, which will help catch potential errors early in the development process.

examples/minimal-solidstart/tsconfig.json (3)

5-5: Approve the change to moduleResolution

The change from "node" to "bundler" for moduleResolution is appropriate for projects using modern bundlers. This aligns well with the dependency updates mentioned in the PR objectives and should provide better compatibility with how bundlers handle modules.


13-13: Approve the update to the types array

Updating the types array to include "vinxi/types/client" instead of "vinxi/client" is a positive change. This more specific path likely provides more accurate or comprehensive type definitions for the vinxi package, which should improve type checking and IDE support in the project.


Line range hint 1-19: Overall approval of tsconfig.json changes

The changes made to the tsconfig.json file are consistent with the PR objectives of updating dependencies and making necessary code modifications. Both the moduleResolution and types updates are improvements that should enhance the project's compatibility with modern bundlers and provide better type support. These changes are approved and should contribute positively to the overall update of the solidstart example.

examples/minimal-solidstart/src/app.tsx (3)

3-3: LGTM: Improved import statement

The import statement for FileRoutes has been updated to specify the router submodule. This change improves code clarity and aligns with best practices for module imports.


6-6: LGTM: Added CSS import

The addition of the CSS import for @uploadthing/solid is a good practice. It ensures that the necessary styles are available throughout the application.


Line range hint 1-24: Summary: Changes align with PR objectives

The modifications to this file are consistent with the PR objectives:

  1. The removal of Tailwind CSS is reflected in the absence of related imports.
  2. The addition of @uploadthing/solid/styles.css import aligns with the stated goal of using these styles instead of Tailwind.
  3. The update to the FileRoutes import suggests dependency updates, which is also mentioned in the PR objectives.

These changes contribute to resolving the CI lint issue mentioned in the PR summary by updating dependencies and adjusting import statements accordingly.

examples/minimal-solidstart/src/entry-server.tsx (2)

1-1: LGTM: Added refresh directive for development.

The addition of // @refresh reload is a good practice for Solid.js applications. This directive ensures that the module is fully reloaded when changes are detected during development, which can help prevent stale code issues.


Line range hint 8-13: Verify: Intentional removal of favicon link?

The favicon link (<link rel="icon" href="/favicon.ico" />) has been removed from the <head> section. While this might be intentional if the favicon is being handled differently elsewhere, it's worth confirming to ensure it doesn't negatively impact the user experience.

Could you please confirm if the removal of the favicon link was intentional? If so, how is the favicon being handled now?

Verification successful

Favicon Handling Verified

The favicon link has been intentionally removed from the <head> section of examples/minimal-solidstart/src/entry-server.tsx. Favicon assets are now managed in the following locations:

  • ./docs/src/app/favicon.ico
  • ./examples/minimal-sveltekit/static/favicon.png

These changes ensure that favicon references are appropriately handled within their respective modules, maintaining user experience without the centralized favicon link.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if favicon is referenced elsewhere in the project

# Test 1: Search for favicon references in HTML, JavaScript, or TypeScript files
echo "Searching for favicon references:"
rg --type html --type js --type ts 'favicon' .

# Test 2: Check if a favicon file exists in the public directory
echo "Checking for favicon file in public directory:"
fd -t f 'favicon.(ico|png|svg)' public

Length of output: 725


Script:

#!/bin/bash
# Description: Comprehensive search for favicon references and files across the entire repository

# Test 1: Search for any references to favicon in all file types
echo "Searching for any favicon references in the entire codebase:"
rg 'favicon' .

# Test 2: Locate favicon files regardless of their directory
echo "Locating favicon files in the repository:"
find . -type f \( -iname "favicon.ico" -o -iname "favicon.png" -o -iname "favicon.svg" \)

Length of output: 765

examples/minimal-solidstart/package.json (4)

4-4: LGTM: "type" field repositioned.

The repositioning of the "type" field is acceptable and doesn't affect the package's functionality.


6-6: Please clarify the removal of NODE_ENV from the "dev" script.

The changes to the scripts section look good overall:

  1. A new "version" script has been added, which is helpful for version management with Vinxi.
  2. The "clean" script now includes cleaning the .solid directory, which is appropriate for SolidJS projects.

However, I noticed that the NODE_ENV prefix was removed from the "dev" script. Can you please explain the reasoning behind this change? It might affect how the development environment is set up.

Also applies to: 9-10


13-15: Verify compatibility with updated dependencies.

The dependency updates look good and should bring improvements:

  1. Minor updates for @solidjs/meta, @solidjs/router, and solid-js.
  2. Major version bump for @solidjs/start (0.6.0 to 1.0.6).
  3. Minor version bump for vinxi with a change to caret range (0.3.11 to ^0.4.3).

Please ensure that these updates, especially the major version bump for @solidjs/start, don't introduce any breaking changes to the project. It's recommended to review the changelogs of these packages and test the application thoroughly.

To verify the impact of these updates, you can run:

Also applies to: 17-17, 19-19


20-20: Clarify the removal of devDependencies.

I noticed that the entire "devDependencies" section has been removed. This is unusual for a modern web project and could potentially affect development and build processes. Could you please explain the reasoning behind this change? Are these dependencies now managed differently, or have they been moved elsewhere?

To check if any dev dependencies are still being used, you can run:

Verification successful

To further investigate the status of development dependencies, I need to examine the content of the package.json file. Let's run a script to view its contents:


The absence of devDependencies is intentional in this minimal example.

Upon examining the package.json file, it's clear that the project is using a simplified dependency structure. All required packages, including those typically considered development dependencies, are listed under the "dependencies" section. This approach, while unconventional, can be valid for minimal setups or example projects.

The project relies on tools like vinxi and @solidjs/start, which likely provide many of the development features typically handled by separate devDependencies. This streamlined configuration reduces complexity for demonstration purposes.

If you're using this as a template for a larger project, you might want to consider separating runtime dependencies from development dependencies in the future. However, for this minimal example, the current setup is sufficient and intentional.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for usage of common dev dependencies

# Test: Search for usage of common dev tools in the project
echo "Checking for usage of common dev tools:"
rg -i "(eslint|prettier|typescript|vite|postcss|tailwindcss)"

Length of output: 172526


Script:

#!/bin/bash
# Description: View the content of package.json file

echo "Content of package.json:"
cat examples/minimal-solidstart/package.json

Length of output: 644

examples/minimal-solidstart/src/utils/uploadthing.ts (5)

5-5: LGTM: Import statement updated correctly.

The addition of generateUploader to the import statement is consistent with the new Uploader export added later in the file. This change aligns well with the PR objectives of updating dependencies and making necessary code modifications.


17-18: LGTM: UploadButton and UploadDropzone exports updated correctly.

The UploadButton and UploadDropzone exports have been properly updated to use the new UploadRouter type. These changes are consistent with the router type import modification and align with the PR objectives.


Line range hint 1-21: Overall assessment: Changes are well-implemented and align with PR objectives.

The modifications in this file successfully update the upload functionality to use the new UploadRouter type. All changes are consistent with the PR objectives and the AI-generated summary. The addition of the Uploader export and the updates to existing exports are implemented correctly.

To ensure full compatibility with the rest of the project, please make sure to update any other files that may be using the old OurFileRouter type or the previous versions of these exports.


19-21: LGTM: Uploader export added and createUploadThing updated correctly.

The addition of the Uploader export and the update to createUploadThing are consistent with the overall changes in this PR. These modifications align well with the PR objectives and the shift to the new UploadRouter type.

To ensure the new Uploader export is being utilized correctly in the project, please run the following verification script:

#!/bin/bash
# Search for usage of the new Uploader export
rg --type typescript --type tsx 'import.*Uploader.*from.*uploadthing'

8-8: LGTM: Router type import updated correctly.

The change from OurFileRouter to UploadRouter is consistent with the PR objectives and summary. This update reflects the shift in the underlying routing structure for the upload functionality.

To ensure the import path is correct, please run the following verification script:

examples/minimal-solidstart/src/routes/index.tsx (2)

12-14: LGTM: Enhanced upload logging

The addition of the onUploadBegin handler improves the logging of the upload process by capturing the filename when an upload starts. This change aligns well with the PR objectives and provides more detailed information for debugging and monitoring uploads.


15-16: LGTM: Improved upload completion logging

The addition of the console.log statement in the onClientUploadComplete handler enhances the logging of the upload process. This change provides more detailed information about the upload completion, which is useful for debugging and monitoring. It's consistent with the PR objectives and the other logging improvements.

import "./global.css";

import { MetaProvider, Title } from "@solidjs/meta";
import { MetaProvider } from "@solidjs/meta";
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Document title not set elsewhere

The Title component has been removed from examples/minimal-solidstart/src/app.tsx, and no other instances were found where the document title is set. Please ensure that the document title is appropriately configured elsewhere in the application to maintain SEO and user experience.

Analysis chain

Verify the document title setting

The Title component from @solidjs/meta has been removed. Ensure that the document title is set appropriately elsewhere in the application, as it's important for SEO and user experience.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the document title is set elsewhere in the application

# Test: Search for Title component usage or document.title setting
rg --type typescript --type tsx 'Title|document\.title'

Length of output: 92


Script:

#!/bin/bash
# Description: Check if the document title is set elsewhere in the application

# Test: Search for Title component usage or document.title setting
rg --type ts --type tsx 'Title|document\.title'

Length of output: 77


Script:

#!/bin/bash
# Description: Check if the document title is set elsewhere in the application

# Test: Search for Title component usage or document.title setting in .ts and .tsx files
rg --glob '*.ts' --glob '*.tsx' 'Title|document\.title'

Length of output: 2806

@juliusmarminge
Copy link
Collaborator

@juliusmarminge, this might conflict with #980.

Perfectly fine that update was also a side effect of me realizing the example wasn't working when I was working on something else 😊

If you don't mind adding tailwind back in though, just hard code the path like I did in my PR. I'll look into issue #975 later.

@juraj98
Copy link
Contributor Author

juraj98 commented Sep 24, 2024

@juliusmarminge, I removed tailwind because it doesn't fit the "minimal" part of the example, not because of #975. minimal-expo is the only one "minimal" that has tailwind. minimal-appdir, minimal-astro-react, minimal-nuxt, minimal-pagedir, and minimal-sveltekit all don't have tailwind. Can add it back though.

@juliusmarminge
Copy link
Collaborator

ah, i see your point...

i just love having tailwind installde when needing to test theming overrides. we have many react examples so makes sense for those not to have it but for the one solid example it's nice to have i tihnk

@juraj98
Copy link
Contributor Author

juraj98 commented Sep 24, 2024

Got you. I'll add tailwind back as soon as I figure out this **** common range dependency lint issue 😂

@juliusmarminge
Copy link
Collaborator

juliusmarminge commented Sep 24, 2024

pnpm manypkg fix && pnpm I is your friend for that

Copy link
Contributor

@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 and nitpick comments (1)
packages/solid/package.json (1)

118-118: Consider updating solid-js peerDependency version.

While the devDependency for solid-js has been updated to ^1.8.23, the peerDependency is still set to ^1.5.3. Consider updating the peerDependency to a version range that includes 1.8.23 to ensure compatibility and avoid potential issues for users of this package.

Suggested change:

  "peerDependencies": {
-   "solid-js": "^1.5.3",
+   "solid-js": "^1.5.3 || ^1.8.0",
    "uploadthing": "^7.0.0"
  },

This change maintains backwards compatibility while also allowing for the newer versions.

Also applies to: 129-129

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 047c4b2 and 8fff301.

Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
Files selected for processing (8)
  • examples/with-clerk-appdir/next-env.d.ts (1 hunks)
  • examples/with-clerk-pagesdir/next-env.d.ts (1 hunks)
  • examples/with-drizzle-appdir/next-env.d.ts (1 hunks)
  • examples/with-drizzle-pagesdir/next-env.d.ts (1 hunks)
  • packages/dropzone/package.json (1 hunks)
  • packages/shared/package.json (1 hunks)
  • packages/solid/package.json (1 hunks)
  • packages/uploadthing/package.json (1 hunks)
Files skipped from review due to trivial changes (4)
  • examples/with-clerk-appdir/next-env.d.ts
  • examples/with-clerk-pagesdir/next-env.d.ts
  • examples/with-drizzle-appdir/next-env.d.ts
  • examples/with-drizzle-pagesdir/next-env.d.ts
Additional comments not posted (4)
packages/shared/package.json (1)

49-49: Approved: solid-js dependency update

The update of solid-js from ^1.8.15 to ^1.8.23 aligns with the PR objective of updating dependencies to their latest versions. This minor version update should maintain backwards compatibility.

To ensure we're using the most appropriate version:

  1. Verify if 1.8.23 is the latest stable version of solid-js:

  2. Check for any breaking changes or important updates in the changelog between these versions:

  3. Verify compatibility with other dependencies, particularly those related to Solid.js:

Please review the results of these checks to confirm the appropriateness of this update.

packages/dropzone/package.json (1)

88-88: Approved: solid-js dependency update

The update of solid-js from ^1.8.15 to ^1.8.23 is a minor version bump, which aligns with the PR objective of updating dependencies to their latest versions. This change follows semantic versioning principles and should introduce new features or improvements without breaking changes.

To ensure compatibility, please run the following verification steps:

  1. Check if this update affects any other parts of the package:

  2. Verify if the peerDependencies for solid-js need to be updated as well:

  3. Run tests (if available) to ensure no regressions:

Consider updating the peerDependencies if necessary to reflect this change.

packages/solid/package.json (1)

118-118: Solid.js devDependency version updated.

The solid-js devDependency has been updated from ^1.8.15 to ^1.8.23. This update likely includes bug fixes and improvements.

To ensure this update doesn't introduce any compatibility issues, please run the following script:

This script will fetch the changelog for Solid.js between versions 1.8.15 and 1.8.23, allowing us to review any breaking changes or notable updates that might affect the package.

packages/uploadthing/package.json (1)

176-176: LGTM: Dependency update for solid-js

The update of solid-js from ^1.8.15 to ^1.8.23 is a minor version bump, which typically includes backwards-compatible new features or bug fixes. This is generally a good practice to keep dependencies up-to-date for security and performance reasons.

To ensure this update doesn't introduce any unexpected issues, please run the following verification steps:

  1. Update the dependency in your local environment.
  2. Run the test suite to check for any regressions.
  3. Perform a quick manual test of the components that use solid-js to ensure they still function as expected.

Copy link
Contributor

@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: 4

Outside diff range and nitpick comments (2)
examples/minimal-solidstart/tailwind.config.js (1)

5-8: LGTM: Content configuration is correct, but requires future attention.

The content configuration is correctly set up for a SolidStart project. However, there's a temporary inclusion of @uploadthing/solid package files that needs to be addressed in the future.

To ensure this is not forgotten, would you like me to create a GitHub issue to track the removal of the temporary inclusion once #975 is resolved?

examples/minimal-solidstart/package.json (1)

21-23: LGTM: Node.js version requirement added

Adding the "engines" field with "node": ">=18" is a good practice. It ensures that the project is run on a compatible Node.js version.

Consider updating the project's README or documentation to mention this Node.js version requirement.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 8fff301 and c4e381c.

Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
Files selected for processing (4)
  • examples/minimal-solidstart/package.json (1 hunks)
  • examples/minimal-solidstart/postcss.config.js (1 hunks)
  • examples/minimal-solidstart/src/app.tsx (1 hunks)
  • examples/minimal-solidstart/tailwind.config.js (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • examples/minimal-solidstart/src/app.tsx
Additional comments not posted (6)
examples/minimal-solidstart/postcss.config.js (1)

1-1: LGTM: Updated export style to ES module syntax.

The change from module.exports to export default aligns with modern JavaScript practices and ES module syntax. This is a good improvement that enhances code consistency and readability.

examples/minimal-solidstart/tailwind.config.js (1)

1-3: LGTM: Import and type declaration are correct.

The import of withUt and the JSDoc type annotation for the Tailwind config are correctly implemented. This setup ensures proper TypeScript support and IDE intellisense for the configuration.

examples/minimal-solidstart/package.json (4)

4-4: LGTM: "type" field repositioned

The repositioning of the "type" field is a minor change that doesn't affect functionality. It's likely done for consistency with a specific style guide.


1-27: Overall review summary

Thank you for updating the dependencies and configurations. However, there are a few points that need clarification:

  1. The removal of NODE_ENV=development from the dev script might affect the development environment setup.
  2. There are major version updates in dependencies, especially for @solidjs/start (0.6.0 to 1.0.6).
  3. TypeScript-related devDependencies (@types/node and typescript) have been removed.
  4. Tailwind CSS has been added despite the PR objectives mentioning its removal.

Could you provide an overall explanation of these changes and how they align with the project's goals? Specifically:

  1. How is the development environment being configured without NODE_ENV=development?
  2. Have you verified that the major version updates don't introduce breaking changes?
  3. Is TypeScript no longer used in the project?
  4. What's the final decision regarding Tailwind CSS usage?

This information will help ensure that the changes are consistent with the project's objectives and maintain its stability.


25-27: Clarification needed on devDependencies changes

  1. autoprefixer (^10.4.19) has been added, suggesting the use of PostCSS for CSS processing.
  2. @types/node and typescript have been removed. This might affect TypeScript support in the project.
  3. tailwindcss (^3.4.1) has been added, which contradicts the PR objectives mentioning the removal of Tailwind CSS.

Could you clarify the following:

  1. Why were @types/node and typescript removed? Is TypeScript no longer used in the project?
  2. The PR objectives mentioned removing Tailwind CSS, but it has been added here. What's the final decision regarding Tailwind CSS usage?

To verify these changes, you can run:

#!/bin/bash
# Check for TypeScript usage in the project
fd -e ts

# Check for Tailwind CSS configuration
fd -e js -e ts -e json tailwind.config

# Check for PostCSS configuration
fd -e js -e json postcss.config

# Search for Tailwind CSS usage in the project
rg -i "@tailwind|@apply" --type css --type js --type ts

13-19: Verify compatibility with updated dependencies

Several dependencies have been updated to newer versions:

  • @solidjs/meta: ^0.29.3 -> ^0.29.4
  • @solidjs/router: ^0.12.4 -> ^0.14.5
  • @solidjs/start: ^0.6.0 -> ^1.0.6 (major version change)
  • solid-js: ^1.8.15 -> ^1.8.23
  • vinxi: 0.3.11 -> ^0.4.3 (changed to caret range)

Please ensure that these updates don't introduce breaking changes, especially for @solidjs/start which has a major version change. Also, verify that the application works correctly with these new versions.

To check for potential breaking changes, you can run:

@juliusmarminge juliusmarminge merged commit fbcf837 into pingdotgg:main Sep 24, 2024
21 checks passed
@juraj98 juraj98 deleted the solidstart-2 branch September 24, 2024 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants