780 vite update dependencies by bourgeoa · Pull Request #794 · SolidOS/solid-ui

bourgeoa · 2026-06-14T16:46:02Z

No description provided.

timea-solid · 2026-06-15T09:05:09Z

I am not sure what to make if this PR. It is containing work from other PRs?

NoelDeMartin · 2026-06-15T10:07:46Z

@timea-solid Yes, it was built from my working branch in #787. The commits are displayed as new commits because I rewrote history addressing some feedback before merging the PR. I guess @bourgeoa can cherry-pick his latest commit on top of main now, or reapply the changes, etc. But the last commit in the PR is what we should be looking at. I'll wait until he resolves the conflicts before adding my review though.

bourgeoa · 2026-06-15T14:38:21Z

@NoelDeMartin
rebase done
local failing test resolved test/unit/login/login.test.ts
build fails on solid-logic as expected

NoelDeMartin · 2026-06-15T14:51:54Z

@bourgeoa The rebase didn't work as expected, because you've rebased the branch with my previous commits. If you look at the PR, it says there are 14 new commits but there should only be two (or maybe one, I think I already fixed that test in #796, I guess whichever gets merged first wins xD).

Instead of a rebase, you should cherry-pick or apply your changes from a clean staging checkout. If you have issues doing that let me know and we can jump on a call and I help you do it.

bourgeoa · 2026-06-15T17:19:50Z

@NoelDeMartin
The issue was with git push

git fetch origin
git reset --hard origin/staging
git cherry-pick fe7f0f57ea468ab7c1494aeaf166b47aeb47d9e4
git push --force-with-lease

I had to fix test test/unit/login/login.test.ts

NoelDeMartin

Overall I think it's fine to merge this after addressing a couple of comments I added, just some minor things about dependencies.

NoelDeMartin · 2026-06-16T07:07:42Z

+declare module '~icons/*'
+
+// CommonJS `module` global used by iconBase.ts for script URI detection
+declare var module: { scriptURI?: string } | undefined


Is this necessary? The only place I've found this being used is something that is casting module as any. Which isn't great either, but since this PR doesn't remove the any, it doesn't fix anything anyways.

In any case, the file using this (src/lib/iconBase.ts) should disappear once we migrate everything to unplugin-icons, so I think it'd be fine leaving the anys for now.

This was needed for typescript 6

NoelDeMartin · 2026-06-16T07:11:08Z

 		"pane-registry": "^3.1.1",
 		"solid-namespace": "^0.5.4",
-		"tailwindcss": "^4.3.0",
+		"tailwindcss": "^4.3.1",


We shouldn't update the tailwind dependency because we have a patch for the 4.3.0 version. In fact, I think we should update this to lock the dependency to 4.3.0, rather than allowing any 4.x.

Where is your patch (for information.
I generally don't agree to lock dependencies.
We should see tests failing when upgrading.

The patch is in patches/tailwindcss+4.3.0.patch, it's applied using patch-package.

You can upgrade the dependency if you want, but we should then update the patch to say 4.3.1 and make sure it's applied correctly.

In this case, this dependency is just importing a reset CSS file, so I think it should be pretty safe to lock to a particular version. This is part of the issues with dependabot and all the automated vulnerability checks... Maybe TailwindCSS will have some vulnerability reported some day, but I doubt our use of the library will be affected. I like this post by Dan Abramov talking about it: npm audit: Broken by Design.

Also, about seeing tests fail when upgrading... as I mentioned, this dependency is only used for CSS. It's very difficult to catch CSS regressions in tests, unless we do visual snapshot testing or something like that, and I don't think we're ready to do that yet (we don't even have E2E tests).

NoelDeMartin · 2026-06-16T07:12:06Z

-		"react-dom": "^17.0.2",
-		"react-is": "^17.0.2",
-		"storybook": "10.4.2",
+		"react": "^19.2.7",


I wonder why we even have React as a dependency 🤔. If it's because of storybook, maybe it could be installed as a transitive dependency instead? I guess we should be able to remove it from here.

NoelDeMartin · 2026-06-16T07:14:12Z

+		"storybook": "10.4.4",
 		"typedoc": "^0.28.19",
-		"typescript": "^5.9.3",
+		"typescript": "^6.0.3",


Any reason why we're upgrading the Typescript version? I'm all for it, but this may not be a trivial change, given that it's a new major version. Did you check if there are any breaking changes or something that could affect us?

The reason for updating dependencies: you introduces a big change with vite.
The other reason is that in the past we were enable to cope with the flow of security dependency updates.
With AI actually it is worse.

I always npm run prepublishOnly that should contains :

all builds

all tests

lint and typecheck

eventually doc, storybook, ....

Let's talk about this in tomorrow's weekly meeting.

bourgeoa requested a review from NoelDeMartin June 14, 2026 16:46

bourgeoa added this to SolidOS NLNet UI Jun 14, 2026

bourgeoa assigned bourgeoa and NoelDeMartin and unassigned bourgeoa Jun 14, 2026

bourgeoa moved this to In review in SolidOS NLNet UI Jun 14, 2026

bourgeoa moved this from In review to In progress in SolidOS NLNet UI Jun 14, 2026

bourgeoa mentioned this pull request Jun 14, 2026

#780 Migrate to Vite, Vitest, and Storybook 10 #787

Merged

bourgeoa removed the request for review from NoelDeMartin June 14, 2026 17:12

timea-solid approved these changes Jun 15, 2026

View reviewed changes

bourgeoa added 2 commits June 15, 2026 18:52

update/remove dependencies

9933c3c

fix:test - remove the obsolete authSession.info spy

66cbb82

bourgeoa force-pushed the 780-vite-update-dependencies branch from 1ccdcfa to 66cbb82 Compare June 15, 2026 17:10

NoelDeMartin requested changes Jun 16, 2026

View reviewed changes

resolved review

896632e

NoelDeMartin moved this from In progress to In review in SolidOS NLNet UI Jun 16, 2026

Conversation

bourgeoa commented Jun 14, 2026

Uh oh!

timea-solid commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NoelDeMartin commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bourgeoa commented Jun 15, 2026

Uh oh!

NoelDeMartin commented Jun 15, 2026

Uh oh!

bourgeoa commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NoelDeMartin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bourgeoa Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

timea-solid commented Jun 15, 2026 •

edited

Loading

NoelDeMartin commented Jun 15, 2026 •

edited

Loading

bourgeoa commented Jun 15, 2026 •

edited

Loading

bourgeoa Jun 16, 2026 •

edited

Loading