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

feat: add root project import in import_map #471

Closed
wants to merge 16 commits into from

Conversation

xstevenyung
Copy link
Contributor

No description provided.

init.ts Outdated Show resolved Hide resolved
@lucacasonato
Copy link
Member

lucacasonato commented Jul 15, 2022

Can you also change the code of the initialized project to make use of @/?

@xstevenyung
Copy link
Contributor Author

good call.

updated 👍

@digitaldesigndj
Copy link
Contributor

digitaldesigndj commented Sep 8, 2022

I ❤️ "@/": "./"

Already using this on my projects. Would love to see it become standard. Only confused about the new preact-signals import on fresh 1.1.0 why is it prefixed with @?

@xstevenyung
Copy link
Contributor Author

It seems like there is some issue with this change on Windows. I think this is unrelated of this PR

@thesobercoder
Copy link

@xstevenyung I created a new project and added "@/": "./". It works perfectly on Windows!

image

@iuioiua
Copy link
Contributor

iuioiua commented May 16, 2023

Tested and working on macOS! I do believe that this change causes the issue on Windows. Is someone with a Windows machine able to troubleshoot?

@iuioiua
Copy link
Contributor

iuioiua commented May 24, 2023

Tested and working on macOS! I do believe that this change causes the issue on Windows. Is someone with a Windows machine able to troubleshoot?

@ayame113, any chance you'd be able to look at this?

@ayame113
Copy link
Contributor

I will look into this later! please wait a little bit!

@ayame113
Copy link
Contributor

Unfortunately I can't reproduce it locally. Can you try rerunning the test and see if this happens again?

@iuioiua iuioiua marked this pull request as draft May 25, 2023 23:32
@iuioiua
Copy link
Contributor

iuioiua commented May 26, 2023

After some debugging, it appears when import Counter from "../islands/Counter.tsx"; is replaced with import Counter from "@/islands/Counter.tsx"; in /init.ts, CI fails on Windows. I'm not sure why.

@ayame113
Copy link
Contributor

ayame113 commented May 26, 2023

I've been debugging this for several days to get to the cause of the CI failure. I think I may have figured out the cause. I'd like to create a minimal repro example and then post the details here.

@ayame113
Copy link
Contributor

This is due to a 20 year old Windows backwards compatibility feature and a Deno CLI bug related to it.

Deno CLI bug for short file names

Windows has a feature called "short file names" for backward compatibility with Windows 95.
This is a feature that creates short 8-character aliases for long paths or paths with spaces.

https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-2000-server/cc976806(v=technet.10)

# Alias for `test dir` is `TESTDI~1`.
C:\Users\ayame\work>dir /x
2023/05/26  15:32    <DIR>                       .
2023/05/09  08:52    <DIR>                       ..
2023/05/26  15:35    <DIR>          TESTDI~1     test dir

Deno CLI has a bug related to short file names.
This only happens when combining deno.json, import-maps with relative paths, and short file names.

The following example works fine.

C:\Users\ayame\work>cd "./test dir"
C:\Users\ayame\work\test dir>type import-map.json
{
  "imports": {
    "@/": "./"
  }
}
C:\Users\ayame\work\test dir>type parent.ts
import "@/child.ts";
import "./child.ts";
C:\Users\ayame\work\test dir>type child.ts
console.log("hello from child.ts!");
C:\Users\ayame\work\test dir>deno info --import-map=import-map.json ./parent.ts
local: C:\Users\ayame\work\test dir\parent.ts
emit: C:\Users\ayame\AppData\Local\deno\gen\file\C\Users\ayame\work\test dir\parent.ts.js
type: TypeScript
dependencies: 1 unique
size: 82B

file:///C:/Users/ayame/work/test%20dir/parent.ts (44B)
├── file:///C:/Users/ayame/work/test%20dir/child.ts (38B)
└── file:///C:/Users/ayame/work/test%20dir/child.ts *
C:\Users\ayame\work\test dir>deno run --import-map=import-map.json ./parent.ts
hello from child.ts!

Next, move the current directory to the directory with the short file name. This also still works fine.

C:\Users\ayame\work\test dir>cd ../TESTDI~1
C:\Users\ayame\work\TESTDI~1>dir

 C:\Users\ayame\work\TESTDI~1

2023/05/26  15:44    <DIR>          .
2023/05/26  15:32    <DIR>          ..
2023/05/26  15:32                38 child.ts
2023/05/26  03:15                43 import-map.json
2023/05/26  03:20                44 parent.ts
C:\Users\ayame\work\TESTDI~1>deno info --import-map=import-map.json ./parent.ts
local: C:\Users\ayame\work\TESTDI~1\parent.ts
emit: C:\Users\ayame\AppData\Local\deno\gen\file\C\Users\ayame\work\TESTDI~1\parent.ts.js
type: TypeScript
dependencies: 1 unique
size: 82B

file:///C:/Users/ayame/work/TESTDI~1/parent.ts (44B)
├── file:///C:/Users/ayame/work/TESTDI~1/child.ts (38B)
└── file:///C:/Users/ayame/work/TESTDI~1/child.ts *
C:\Users\ayame\work\TESTDI~1>deno run --import-map=import-map.json ./parent.ts
hello from child.ts!

Now, creating deno.json messes up dependency resolution.
child.ts is loaded twice. This is the bug.

C:\Users\ayame\work\TESTDI~1>type deno.json
{
  "importMap": "./import-map.json"
}
# ↓`test%20dir/child.ts` and `TESTDI~1/child.ts` are included in dependencies.
C:\Users\ayame\work\TESTDI~1>deno info ./parent.ts
local: C:\Users\ayame\work\TESTDI~1\parent.ts
emit: C:\Users\ayame\AppData\Local\deno\gen\file\C\Users\ayame\work\TESTDI~1\parent.ts.js
type: TypeScript
dependencies: 2 unique
size: 120B

file:///C:/Users/ayame/work/TESTDI~1/parent.ts (44B)
├── file:///C:/Users/ayame/work/test%20dir/child.ts (38B)
└── file:///C:/Users/ayame/work/TESTDI~1/child.ts (38B)
# ↓The hello message will be displayed twice.(bug)
C:\Users\ayame\work\TESTDI~1>deno run ./parent.ts
hello from child.ts!
hello from child.ts!

Temporary directory in GitHub Actions

The temporary directory in GitHub Actions is set to short file name by default.
It should be C:\Users\runneradmin\AppData\Local\Temp, but the actual value is C:\Users\RUNNER~1\AppData\Local\Temp. (This is exactly the phenomenon described in actions/runner-images#712.)

Fresh's tests/cli_test.ts uses this temporary directory. It will run init.ts inside the temporary directory and create a fresh project inside.

fresh/tests/cli_test.ts

Lines 40 to 44 in 7f3fd51

Deno.test({
name: "fresh-init",
async fn(t) {
// Preparation
const tmpDirName = await Deno.makeTempDir();

Fresh island detection in tests

During the test run, inside the temporary directory there is deno.json and import-map with relative path.("@/": "./")
The temporary directory is a short file name. The conditions for the bug to occur are now met. At this point, running the deno info command shows the following:

file:///C:/Users/RUNNER~1/AppData/Local/Temp/e77a1c79/main.ts (316B)
├─┬ .....
└─┬ file:///C:/Users/RUNNER~1/AppData/Local/Temp/e77a1c79/fresh.gen.ts (672B)
  ├── ....
  ├─┬ file:///C:/Users/RUNNER~1/AppData/Local/Temp/e77a1c79/routes/index.tsx (567B)
  │ ├── ....
  │ └─┬ file:///C:/Users/runneradmin/AppData/Local/Temp/e77a1c79/islands/Counter.tsx (550B) // !!!
  │   └─ ...
  └─┬ file:///C:/Users/RUNNER~1/AppData/Local/Temp/e77a1c79/islands/Counter.tsx (550B) // !!!
    ├── ...

The Counter component is imported twice from separate paths (runneradmin and RUNNER~1). Unfortunately, this has a negative impact on island detection.
Inside render.ts, fresh detects the islands by comparing the Counter component references.

fresh/src/server/render.ts

Lines 367 to 373 in 7f3fd51

const island = ISLANDS.find((island) => island.component === originalType);
if (island) {
if (ignoreNext) {
ignoreNext = false;
return;
}
ENCOUNTERED_ISLANDS.add(island);

Here we have a Counter component imported from fresh.gen.ts and a Counter component imported from routes/index.ts. Comparing these references yields false, so the island is not detected.

import Counter1 from "@/islands/Counter.tsx";
import Counter2 from "./islands/Counter.tsx";
Counter1 !== Counter2; // due to bugs.

Test failure

Failed to find island and the value of variable ENCOUNTERED_ISLANDS is 0. As a result, the page is considered static and no JS is delivered to the browser.

Telling the puppeteer to click doesn't work the counter.
And the assertion at line 316 in cli_test.ts fails.

fresh/tests/cli_test.ts

Lines 311 to 316 in 7f3fd51

await buttonPlus?.click();
await delay(100);
counterValue = await counter?.evaluate((el) => el.textContent);
assert(counterValue === "4");

After that, subsequent tests will also randomly fail because resource cleanup is not done.

How to respond

As for the deno.json bug, I'm not familiar with Rust so I'm not sure.
Could it be the call to path.canonicalize on line 772 of cli/args/config_file.rs that is the problem?

ref: rust-lang/rust#59117, rust-lang/rust#76586,

https://github.com/denoland/deno/blob/25cbd97ab7ef1866e58238f1c28ec0d86062aee8/cli/args/config_file.rs#L772

Also, a possible workaround on the Fresh side, as described by actions/runner-images#712 (comment), seems to be to override the TMP/TEMP environment variables in the GitHub Actions settings.
Doing this will prevent short file names from being used for temporary directories, so CI will work.

# .github\workflows\ci.yml
      # Workaround for using temporary directory on Windows
      # https://github.com/actions/runner-images/issues/712
      - name: Set the TMP environment variable (Windows)
        if: startsWith(matrix.os, 'windows')
        run: echo "TMP=$env:USERPROFILE\AppData\Local\Temp" >> $env:GITHUB_ENV
      - name: Set the TEMP environment variable (Windows)
        if: startsWith(matrix.os, 'windows')
        run: echo "TEMP=$env:USERPROFILE\AppData\Local\Temp" >> $env:GITHUB_ENV

There are the result of debugging on https://github.com/ayame113/fresh/commits/win-failed-test branch.

@iuioiua
Copy link
Contributor

iuioiua commented Jun 5, 2023

Excellent troubleshooting, @ayame113! Very helpful. I've applied the workaround to CI.

Now, we're running into an issue caused by deno.json being in a parent directory instead of the standard root. I guess we could dynamically set the @/ import during the execution of the init script. Sound good, @lucacasonato?

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.

9 participants