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.

Sign up for GitHub

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

Jump to bottom

Windows #3

Merged
merged 13 commits into from
Sep 25, 2023
Merged

Windows #3

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
fe07092
fix windows related path issues
edivados Sep 9, 2023
db94dbb
Merge branch 'main' into windows
edivados Sep 22, 2023
d7ed40d
more path issues
edivados Sep 22, 2023
51b6d84
Merge branch 'main' into windows
edivados Sep 22, 2023
716c7eb
Merge branch 'main' into windows
edivados Sep 23, 2023
0244151
refactor base file system router globbing
edivados Sep 24, 2023
54f1a15
Merge branch 'main' into windows
edivados Sep 24, 2023
e954b8b
add pathe back to router dev plugins
edivados Sep 24, 2023
c24680c
update pnpm-lock.yaml
edivados Sep 24, 2023
acf955c
remove stuff from other branch
edivados Sep 24, 2023
ad5d6f1
base file system router glob pattern again
edivados Sep 25, 2023
fc52fd2
Merge branch 'main' into windows
edivados Sep 25, 2023
d3b8eed
fix hmr
edivados Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions packages/vinxi/lib/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ import {
} from "h3";
import { createRequire } from "module";
import { build, copyPublicAssets, createNitro } from "nitropack";
import { join } from "path";
import { relative } from "pathe";
import { join, relative } from "pathe";

import { writeFileSync } from "node:fs";

Expand All @@ -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");
Copy link
Contributor Author

@edivados edivados Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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});`;

const { fileURLToPath } = await import("url");
for (const router of app.config.routers) {
if ("compile" in router) {
Expand Down Expand Up @@ -111,7 +110,7 @@ export async function createBuild(app, buildConfig) {
? "virtual:#vinxi/handler"
: relative(app.config.root, router.handler)
].file,
).replace(/\\/g, "\\\\");
);

return [
{
Expand Down Expand Up @@ -548,7 +547,7 @@ function handerBuild() {
"Invalid router",
);
const { builtinModules } = await import("module");
const { join } = await import("path");
const { join } = await import("pathe");
const input = await getEntries(inlineConfig.router);
return {
build: {
Expand Down Expand Up @@ -587,7 +586,7 @@ function browserBuild() {
inlineConfig.router && inlineConfig.router.mode !== "static",
"Invalid router",
);
const { join } = await import("path");
const { join } = await import("pathe");
return {
build: {
rollupOptions: {
Expand Down
2 changes: 1 addition & 1 deletion packages/vinxi/lib/dev-server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
} from "h3";
import { createNitro } from "nitropack";

import { join } from "node:path";
import { join } from "pathe";
Copy link
Contributor Author

@edivados edivados Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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});`;
}

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@edivados edivados Sep 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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: ✔️

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@edivados edivados Sep 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@edivados edivados Sep 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

import { fileURLToPath } from "node:url";
import { isMainThread } from "node:worker_threads";

Expand Down
4 changes: 2 additions & 2 deletions packages/vinxi/lib/manifest/collect-styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"use strict";

import { isBuiltin } from "node:module";
import { join, resolve } from "node:path";
import { join, resolve } from "pathe";

async function getViteModuleNode(vite, file, ssr) {
if (file.startsWith("node:") || isBuiltin(file)) {
Expand All @@ -19,7 +19,7 @@ async function getViteModuleNode(vite, file, ssr) {

const id = resolvedId.id;

const normalizedPath = resolve(id).replace(/\\/g, "/");
const normalizedPath = resolve(id);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


try {
let node = await vite.moduleGraph.getModuleById(normalizedPath);
Expand Down
4 changes: 2 additions & 2 deletions packages/vinxi/lib/manifest/dev-server-manifest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export function createDevManifest(app) {
} else {
return {
output: {
path: absolutePath,
path: join(router.base, "@fs", absolutePath),
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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),

},
};
}
Expand Down Expand Up @@ -173,7 +173,7 @@ export function createDevManifest(app) {
];
},
output: {
path: absolutePath,
path: join(router.base, "@fs", absolutePath),
Copy link
Contributor Author

@edivados edivados Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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
);

},
};
}
Expand Down
2 changes: 1 addition & 1 deletion packages/vinxi/lib/plugins/routes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { relative } from "node:path";
import { relative } from "pathe";
import { fileURLToPath } from "node:url";
Copy link
Contributor Author

@edivados edivados Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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,


export function routes() {
Expand Down
Loading