Windows #3

edivados · 2023-09-22T12:38:43Z

I have been playing around with the vinxi branch on solid-start and ran into the following issues.

dev and build not working because of path \ issues
hmr tests (here) failing because new routes (file) are not picked up

I got the first point to work with these changes but I am not sure on some and am still testing. Added a bit of context to some changes.
Right now this is more of a "hey there are issues...". Feel free to fix it yourself and close this PR.

vercel · 2023-09-22T12:38:49Z

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

Name	Status	Preview	Comments	Updated (UTC)
vinxi	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 25, 2023 5:47pm

edivados · 2023-09-22T12:48:50Z

packages/vinxi/lib/manifest/collect-styles.js

@@ -19,7 +19,7 @@ async function getViteModuleNode(vite, file, ssr) {

 	const id = resolvedId.id;

-	const normalizedPath = resolve(id).replace(/\\/g, "/");
+	const normalizedPath = resolve(id);


@solid-refresh gets converted to C:/@solid-refresh causing errors

edivados · 2023-09-22T13:06:03Z

packages/vinxi/lib/plugins/routes.js

@@ -1,4 +1,4 @@
-import { relative } from "node:path";
+import { relative } from "pathe";


Manifest lookups in build would fail because of \.

vinxi/packages/vinxi/lib/plugins/routes.js

Line 77 in 505dd51

src: isBuild ? relative(root, buildId) : buildId,

edivados · 2023-09-22T13:14:03Z

packages/vinxi/lib/build.js

@@ -34,7 +33,7 @@ const require = createRequire(import.meta.url);
 */
 export async function createBuild(app, buildConfig) {
 	const { existsSync, promises: fsPromises, readFileSync } = await import("fs");
-	const { join } = await import("path");
+	const { join } = await import("pathe");


These imports would fail because of characters being escaped generating paths like C:goodjobwindows

vinxi/packages/vinxi/lib/build.js

Lines 457 to 460 in 505dd51

import middleware from "${join(config.router.root, config.router.middleware)}";

import handler from "${join(config.router.root, config.router.handler)}";

import { eventHandler } from "vinxi/runtime/server";

export default eventHandler({ onRequest: middleware.onRequest, onBeforeResponse: middleware.onBeforeResponse, handler});`;

edivados · 2023-09-22T13:17:54Z

packages/vinxi/lib/manifest/dev-server-manifest.js

@@ -173,7 +173,7 @@ export function createDevManifest(app) {
 											];
 										},
 										output: {
-											path: absolutePath,
+											path: join(router.base, "@fs", absolutePath),


Not sure on this one but seems to work. If I remember it right dynamic import in lazy route would not work.

vinxi/packages/vinxi-solid/lazy-route.jsx

Lines 20 to 22 in 505dd51

const mod = await import(

/* @vite-ignore */ manifest.inputs[component.src].output.path

);

edivados · 2023-09-22T13:25:26Z

packages/vinxi/lib/dev-server.js

@@ -8,7 +8,7 @@ import {
 } from "h3";
 import { createNitro } from "nitropack";

-import { join } from "node:path";
+import { join } from "pathe";


Same thing here as in build.js. Characters being escaped because of \.

vinxi/packages/vinxi/lib/dev-server.js

Lines 156 to 160 in 505dd51

import middleware from "${join(config.router.root, config.router.middleware)}";

import handler from "${join(config.router.root, config.router.handler)}";

import { eventHandler } from "vinxi/runtime/server";

export default eventHandler({ onRequest: middleware.onRequest, onBeforeResponse: middleware.onBeforeResponse, handler});`;

}

So, using pathe helps in these Windows situations? if yes, I am so in with all these changes. I have a tough time maintaining windows coz I don't have access to a machine

At the very least it should not cause problems for linux since pathe uses /.
The only change I am unsure about is path: join(router.base, "@fs", absolutePath) on dev-server-manifest.js.

From a quick test it looks like as long things go through vite/rollup it's fine to import with C:/some/path
I guess vite/rollup goes through Windows which supports / but node does whatever node does.

import test from "C:/Users/Davide/Desktop/wpath/w/dep.js"

Node: ❌ not supported
Vite/rollup: ✔️

import test from "C:\\Users\\Davide\\Desktop\\wpath\\w\\dep.js"

Node: ❌ not supported
Vite/rollup: ✔️

import test from "file:///C:/Users/Davide/Desktop/wpath/w/dep.js"

Node: ✔️
Vite/rollup: ❌ rollup can't resolve

import test from "/Users/Davide/Desktop/wpath/w/dep.js" import test from "/C:/Users/Davide/Desktop/wpath/w/dep.js"

Node: ✔️
Vite/rollup: ✔️

So you have tested these changes on windows? and they are all working good? is there anything else that's not working on windows?

TBH my brain hurts when I try to decipher this windows/linux path differences, I never know what to do so this a big saving grace for me if you have got it all working

It's honestly quite difficult to tell if everything actually works. I am fighting with a lot of inconsistency. Primarily used the tests here and the bare example on the solid-start/vinxi branch. The example on the solid-start repo is not reliable as it had other issues unrelated to vinxi.

If there is anything specific you want me to run on my machine let me know.
I just run the tests again on a code space.

Windows windows branch: two additional tests fail (on main none pass). Hmm...

3 failed [chromium] › basic-dev.test.ts:40:3 › basic dev › api ========================================== [chromium] › basic-prod.test.ts:40:3 › basic build › api ======================================= [chromium] › hmr.test.ts:81:3 › hmr › hmr api ================================================== 6 passed (39s)

Linux windows branch:

One test just randomly fails if I run it enough times. It also looks like it's not releasing the ports?

1) [chromium] › hmr.test.ts:81:3 › hmr › hmr api ================================================= Error: expect(received).toBe(expected) // Object.is equality - Expected - 1 + Received + 7 - Hello world + <!DOCTYPE html><html lang="en"><head><link rel="icon" href="/favicon.ico"/><script type="module"> + import RefreshRuntime from "/_build/@react-refresh" + RefreshRuntime.injectIntoGlobalHook(window) + window.$RefreshReg$ = () => {} + window.$RefreshSig$ = () => (type) => type + window.__vite_plugin_react_preamble_installed__ = true + </script><script type="module" src="/_build/@vite/client"></script></head><body><section><h1 data-test-id="content">Hello from Vinxi</h1><div><button data-test-id="button">Click me</button><span data-test-id="count">0</span></div></section></body></html><script>window.manifest = {}</script><script type="module" src="/_build/@fs/workspaces/vinxi/test/.fixtures/hmr-dupkt25h3h8/app/entry-client.tsx" async=""></script> 83 | prettyHtml(`<h1 data-test-id="content">Hello from Vinxi too</h1>`), 84 | ); > 85 | }); | ^ 86 | 87 | test("client hmr", async ({ page }) => { 88 | let app = new PlaywrightFixture(appFixture, page); at file:///workspaces/vinxi/test/hmr.test.ts:85:30 1 failed [chromium] › hmr.test.ts:81:3 › hmr › hmr api ================================================== 8 passed (35s)

I will take a look a it again later today with a fresh mind.

~~Looks like more escaping is going on.~~
Edit: Ignore the sentence above. I switched to main branch on my machine. Definitely need a break.

Yeah man you are doing amazing work here. This is the biggest overall issue here.. the differences between the OSes when it comes to paths. I can get on a google meet if you want to chat the issues. (my discord username is nksaraf if you want to get in touch there)

Thanks for the offer, I am on the solidjs discord as edivado. I may send a message your way on Discord in case I get stuck or something is super unclear if that's cool with you.

Btw the two failed tests are the glob/micromatch failing the isRoute check. I am not familiar with fast-glob and micromatch so am currently looking at their repos and reading up on them.

Backslash

To solve this, you might be inspired to do something like 'foo\*'.replace(/\/g, '/'), but this causes another, potentially much more serious, problem.

Not elaborating further is great... 🤣

After that, it's just the hmr issue that's left. It's a bit of a weird one. Last time I checked file creation was being picked up but not the edits in some cases.

nksaraf · 2023-09-23T09:17:35Z

Can you run pnpm i and push with an updated pnpm-lock file so that we can run the tests?

edivados · 2023-09-23T09:25:54Z

Can you run pnpm i and push with an updated pnpm-lock file so that we can run the tests?

done.

packages/vinxi/lib/file-system-router.js

nksaraf · 2023-09-25T19:29:25Z

packages/vinxi/lib/manifest/dev-server-manifest.js

@@ -51,7 +51,7 @@ export function createDevManifest(app) {
 								} else {
 									return {
 										output: {
-											path: absolutePath,
+											path: join(router.base, "@fs", absolutePath),


this change will not in some modes: this change will not work, on the server we don't want to use @fs links. those are for the client. this is being used with the server actions setup. I should add a test for that

@nksaraf What about LN 175? Should that also be reverted in that case? I did this in two places.

vinxi/packages/vinxi/lib/manifest/dev-server-manifest.js

Line 175 in 2fa85e2

path: join(router.base, "@fs", absolutePath),

nksaraf · 2023-09-25T20:04:35Z

Heyy I merged this maybe a bit eagerly but its good to get this stuff tested quickly. I added another test with some rsc stuff. can you get it to work for windows?

edivados added 4 commits September 9, 2023 20:05

fix windows related path issues

fe07092

Merge branch 'main' into windows

db94dbb

more path issues

d7ed40d

Merge branch 'main' into windows

51b6d84

vercel bot deployed to Preview September 22, 2023 12:39 View deployment

edivados commented Sep 22, 2023

View reviewed changes

Merge branch 'main' into windows

716c7eb

vercel bot deployed to Preview September 23, 2023 09:23 View deployment

edivados added 4 commits September 24, 2023 17:40

refactor base file system router globbing

0244151

Merge branch 'main' into windows

54f1a15

add pathe back to router dev plugins

e954b8b

update pnpm-lock.yaml

c24680c

vercel bot deployed to Preview September 24, 2023 18:21 View deployment

edivados commented Sep 24, 2023

View reviewed changes

packages/vinxi/lib/file-system-router.js Outdated Show resolved Hide resolved

edivados commented Sep 24, 2023

View reviewed changes

packages/vinxi/lib/file-system-router.js Outdated Show resolved Hide resolved

remove stuff from other branch

acf955c

vercel bot deployed to Preview September 24, 2023 19:31 View deployment

base file system router glob pattern again

ad5d6f1

vercel bot deployed to Preview September 25, 2023 12:12 View deployment

edivados added 2 commits September 25, 2023 14:52

Merge branch 'main' into windows

fc52fd2

fix hmr

d3b8eed

vercel bot deployed to Preview September 25, 2023 17:47 View deployment

nksaraf reviewed Sep 25, 2023

View reviewed changes

nksaraf merged commit cf81fea into nksaraf:main Sep 25, 2023
3 checks passed

edivados deleted the windows branch September 26, 2023 06:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows #3

Windows #3

edivados commented Sep 22, 2023 •

edited

Loading

vercel bot commented Sep 22, 2023 •

edited

Loading

edivados Sep 22, 2023

edivados Sep 22, 2023 •

edited

Loading

edivados Sep 22, 2023 •

edited

Loading

edivados Sep 22, 2023 •

edited

Loading

edivados Sep 22, 2023 •

edited

Loading

nksaraf Sep 22, 2023

edivados Sep 23, 2023 •

edited

Loading

nksaraf Sep 23, 2023

nksaraf Sep 23, 2023

edivados Sep 23, 2023 •

edited

Loading

edivados Sep 23, 2023 •

edited

Loading

nksaraf Sep 23, 2023

edivados Sep 23, 2023

nksaraf commented Sep 23, 2023

edivados commented Sep 23, 2023

nksaraf Sep 25, 2023

edivados Sep 26, 2023

nksaraf commented Sep 25, 2023

		@@ -1,4 +1,4 @@
		import { relative } from "node:path";
		import { relative } from "pathe";

	import middleware from "${join(config.router.root, config.router.middleware)}";
	import handler from "${join(config.router.root, config.router.handler)}";
	import { eventHandler } from "vinxi/runtime/server";
	export default eventHandler({ onRequest: middleware.onRequest, onBeforeResponse: middleware.onBeforeResponse, handler});`;

	const mod = await import(
	/* @vite-ignore */ manifest.inputs[component.src].output.path
	);

Windows #3

Windows #3

Conversation

edivados commented Sep 22, 2023 • edited Loading

vercel bot commented Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

edivados Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

edivados Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

edivados Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

edivados Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edivados Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edivados Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

edivados Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nksaraf commented Sep 23, 2023

edivados commented Sep 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nksaraf commented Sep 25, 2023

edivados commented Sep 22, 2023 •

edited

Loading

vercel bot commented Sep 22, 2023 •

edited

Loading

edivados Sep 22, 2023 •

edited

Loading

edivados Sep 22, 2023 •

edited

Loading

edivados Sep 22, 2023 •

edited

Loading

edivados Sep 22, 2023 •

edited

Loading

edivados Sep 23, 2023 •

edited

Loading

edivados Sep 23, 2023 •

edited

Loading

edivados Sep 23, 2023 •

edited

Loading