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

jsc.paths regression in 1.3.79 #8265

Closed
alexeagle opened this issue Nov 10, 2023 · 14 comments · Fixed by #8297
Closed

jsc.paths regression in 1.3.79 #8265

alexeagle opened this issue Nov 10, 2023 · 14 comments · Fixed by #8297
Assignees
Labels
Milestone

Comments

@alexeagle
Copy link
Contributor

alexeagle commented Nov 10, 2023

Describe the bug

swc re-writes import paths by following symlinks, it seems. I believe this is a regression from one of the changes @realtimetodie made when we originally got SWC working under Bazel.

It now looks like:

require("../../../../../../../private/var/tmp/_bazel_alexeagle/43e97bf6681e7317438fd77d405a1c4b/sandbox/darwin-sandbox/181/execroot/aspect_rules_swc/examples/paths/src/modules/moduleA");

Minimal repro:

  1. clone https://github.com/aspect-build/rules_swc
  2. npx @bazel/bazelisk build --subcommands examples/paths:compile
  3. Open bazel-bin/examples/paths/src/index.js and you'll see the require path is relative
  4. update LATEST_SWC_VERSION = "v1.3.78" to v1.3.79 in swc/repositories.bzl
  5. repeat step 2 to build again
  6. Look at the same index.js file and the require path now contains an absolute path to the root of the filesystem

If you need the minimal repro to not involve Bazel, we can engineer one that creates a similar symlink tree to reproduce the problem without it.

Note, this is my fourth attempt to file an issue here, see prior attempt #8264.

Input code

import { moduleA } from "@modules/moduleA";

moduleA();

Config

{
  "jsc": {
    "parser": {
      "syntax": "typescript"
    },
    "target": "es2021",
    "baseUrl": "./src",
    "paths": {
      "@modules/*": [
        "./modules/*"
      ]
    }
  },
  "module": {
    "type": "commonjs"
  },
  "sourceMaps": true
}

Playground link (or link to the minimal reproduction)

https://github.com/aspect-build/rules_swc/pull/216/files

SWC Info output

swc-darwin-arm64 --version
SWC 0.91.64

Expected behavior

SWC 1.3.78 produces

const _moduleA = require("./modules/moduleA/index");

In #8258 you suggested that we compare with what tsc produces. As I understand it, typescript never attempts to re-write import/require statements. So the result is here

https://github.com/aspect-build/rules_swc/pull/216/files#diff-e3a9eafb3b943b9863d6cea0b348d4ef40bc1b7bb5867a38ac889fcacea0cbf4R3

"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
var moduleA_1 = require("@modules/moduleA");
(0, moduleA_1.moduleA)();

Actual behavior

SWC 1.3.79 produces

const _moduleA = require("../../../../../../../shared/cache/bazel/user_base/09a27d1eeea3b44d1580773090924b4d/sandbox/linux-sandbox/629/execroot/aspect_rules_swc/examples/paths/src/modules/moduleA");

Version

1.3.79

Additional context

In 1.3.78 this worked, so #7852 is the likely culprit, as @kdy1 agrees #8258 (comment)

@kdy1 kdy1 added this to the Planned milestone Nov 10, 2023
@kdy1 kdy1 self-assigned this Nov 10, 2023
@kdy1
Copy link
Member

kdy1 commented Nov 10, 2023

I don't think it's culprit 🤣

@kdy1
Copy link
Member

kdy1 commented Nov 10, 2023

Note that I'll investigate, but if fixing this issue results in something which does not match tsc, I'll not fix this and reject patches. Matching the behavior of tsc is more important than satisfying really specific use case.

@kdy1
Copy link
Member

kdy1 commented Nov 10, 2023

Did you try some newer versions? I think it will error with a more useful error complaining about jsc.baseUrl.

@realtimetodie
Copy link
Contributor

realtimetodie commented Nov 10, 2023

Yes, this is a regression. The tsc compiler does not re-write import/require statements to symbolic links. Actually, if you look into the source code of the tsc compiler and how symbolic links are handled, you won't find anything special, because tsc simply relies on the Node.js runtime's path module.

@kdy1
Copy link
Member

kdy1 commented Nov 10, 2023

I can't reproduce it without Bazel

image

@kdy1
Copy link
Member

kdy1 commented Nov 10, 2023

Yeah actually the TscResolver were written for the bundler, so it patches import path, but I'm not sure if it was a good idea to reuse it for modules transform.

@alexeagle
Copy link
Contributor Author

@kdy1 I can build a repro without Bazel, the difference is that the input files are staged as symlinks.
(because Bazel tries to make builds reproducible by sandboxing the build steps)

@kdy1
Copy link
Member

kdy1 commented Nov 11, 2023

Ah, it would be nice. I need to create a test case without using Bazel

@alexeagle
Copy link
Contributor Author

Repro:

  1. Download swcx from https://github.com/swc-project/swc/releases/tag/v1.3.79 (we don't use npm or node.js under Bazel to run swc since you have the native CLI)

  2. Run this script which emulates a "sandbox" strategy - it makes a tmp dir and symlinks the sources into it to make a more reproducible build just like Bazel does

#!/usr/bin/env bash
set -o errexit -o nounset

TMP=$(mktemp -d)
mkdir -p $TMP/src/modules/moduleA
mkdir -p $TMP/src/modules/moduleB

ln -s $PWD/.swcrc $TMP
ln -s $PWD/src/index.ts $TMP/src
ln -s $PWD/src/modules/moduleA/index.ts $TMP/src/modules/moduleA
ln -s $PWD/src/modules/moduleB/index.ts $TMP/src/modules/moduleB

cd $TMP
ls -alR
~/Downloads/swc-darwin-arm64 compile --source-maps false --config-file .swcrc --out-file src/index.js src/index.ts
grep require src/index.js

Running the script gives this output:

% ./repro.sh 
total 0
drwx------    4 alexeagle  staff    128 Nov 12 11:08 .
drwx------@ 835 alexeagle  staff  26720 Nov 12 11:08 ..
lrwxr-xr-x    1 alexeagle  staff     34 Nov 12 11:08 .swcrc -> /Users/alexeagle/repros/swc/.swcrc
drwxr-xr-x    4 alexeagle  staff    128 Nov 12 11:08 src

./src:
total 0
drwxr-xr-x  4 alexeagle  staff  128 Nov 12 11:08 .
drwx------  4 alexeagle  staff  128 Nov 12 11:08 ..
lrwxr-xr-x  1 alexeagle  staff   40 Nov 12 11:08 index.ts -> /Users/alexeagle/repros/swc/src/index.ts
drwxr-xr-x  4 alexeagle  staff  128 Nov 12 11:08 modules

./src/modules:
total 0
drwxr-xr-x  4 alexeagle  staff  128 Nov 12 11:08 .
drwxr-xr-x  4 alexeagle  staff  128 Nov 12 11:08 ..
drwxr-xr-x  3 alexeagle  staff   96 Nov 12 11:08 moduleA
drwxr-xr-x  3 alexeagle  staff   96 Nov 12 11:08 moduleB

./src/modules/moduleA:
total 0
drwxr-xr-x  3 alexeagle  staff   96 Nov 12 11:08 .
drwxr-xr-x  4 alexeagle  staff  128 Nov 12 11:08 ..
lrwxr-xr-x  1 alexeagle  staff   56 Nov 12 11:08 index.ts -> /Users/alexeagle/repros/swc/src/modules/moduleA/index.ts

./src/modules/moduleB:
total 0
drwxr-xr-x  3 alexeagle  staff   96 Nov 12 11:08 .
drwxr-xr-x  4 alexeagle  staff  128 Nov 12 11:08 ..
lrwxr-xr-x  1 alexeagle  staff   56 Nov 12 11:08 index.ts -> /Users/alexeagle/repros/swc/src/modules/moduleB/index.ts
const _moduleA = require("../../../../../private/var/folders/8n/7cfngjb106sgwb8mr1tytx700000gn/T/tmp.tCjESfPJ/src/modules/moduleA");

Here's a GH repo with the files: https://github.com/alexeagle/swc_8265_repro/

I confirmed that downloading from https://github.com/swc-project/swc/releases/tag/v1.3.78 instead gives the expected require statement.

@kdy1
Copy link
Member

kdy1 commented Nov 16, 2023

I managed to write a test which actually create symlink, and invoke Rust CLI to prevent regression perfectly.

https://github.com/swc-project/swc/pull/8297/files#diff-91e80712ad85568b1133ec3751cb89ec876af4674567542a1c5c506b497e1609

@kdy1
Copy link
Member

kdy1 commented Nov 16, 2023

Feel free to contribute test case to the file.

@alexeagle
Copy link
Contributor Author

Amazing thank you @kdy1 !

I've never written Rust so I don't think you'd want my contribution to that test case. It looks sufficient already though. Does it need more changes before landing?

@kdy1
Copy link
Member

kdy1 commented Nov 16, 2023

It just needs an approval 😄

kdy1 added a commit that referenced this issue Nov 20, 2023
@kdy1 kdy1 modified the milestones: Planned, v1.3.98 Nov 21, 2023
@swc-bot
Copy link
Collaborator

swc-bot commented Dec 21, 2023

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@swc-project swc-project locked as resolved and limited conversation to collaborators Dec 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.