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

fix(swc): Remove import statement canonicalization #6716

Merged
merged 1 commit into from Dec 27, 2022

Conversation

realtimetodie
Copy link
Contributor

Fixes an issue where import statements to symbolic links (symlinks) are canonicalized in generated code and use the absolute form of the path with all intermediate components normalized and symbolic links resolved.

Import resolution was introduced in #1834.

However, resolving symbolic links in generated code is unnecessary. A JavaScript runtime (Node.js, Deno) will resolve import statements to symbolic links at runtime.

The suggested solution introduces a new match guard to determine whether the path is relative and uses the current working directory as a base path. For example, the current working directory is already used as a base path for module resolution

std::env::current_dir()?.join(path)

Actual behavior (generated code, canonicalized import statement from a symbolic link)

const a = require("../../../src/modules/a");

Expected behavior

const a = require("./modules/a");

Related

@realtimetodie
Copy link
Contributor Author

The tests are failing with the same error message. I am not sure how to handle this.

@kwonoj

Error("unknown field `chrome-android`, expected one of `chrome`, `and_chr`, `and_ff`, `op_mob`, `ie`, `edge`,
 `firefox`, `safari`, `node`, `ios`, `samsung`, `opera`, `android`, `electron`, `phantom`, `opera_mobile`, `rhino`, 
 `deno`, `hermes`, `oculus`, `bun`", line: 6, column: 20)', crates/swc_ecma_preset_env/src/corejs3/compat.rs:10:10

@kdy1
Copy link
Member

kdy1 commented Dec 26, 2022

I'll fix CI with another PR

@realtimetodie
Copy link
Contributor Author

Rebased with main

Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

Thank you!


swc-bump:

  • swc

@kdy1 kdy1 enabled auto-merge (squash) December 27, 2022 00:59
@kdy1 kdy1 added this to the Planned milestone Dec 27, 2022
@kdy1 kdy1 merged commit b451fa9 into swc-project:main Dec 27, 2022
@kdy1 kdy1 modified the milestones: Planned, v1.3.25 Jan 5, 2023
@swc-project swc-project locked as resolved and limited conversation to collaborators Feb 4, 2023
@swc-project swc-project unlocked this conversation Feb 10, 2023
@kdy1
Copy link
Member

kdy1 commented Feb 10, 2023

I'm considering reverting this because of #6782.
Do you think this PR fixes any real issues?

kdy1 added a commit to kdy1/swc that referenced this pull request Feb 10, 2023
@kdy1
Copy link
Member

kdy1 commented Feb 10, 2023

I added reverting commit to #6930

@realtimetodie
Copy link
Contributor Author

realtimetodie commented Feb 10, 2023

Why was this reverted? @kdy1

This breaks rules_swc @alexeagle

@kdy1
Copy link
Member

kdy1 commented Feb 10, 2023

Because this PR is breaking a valid app and there's no test case in this PR

kdy1 added a commit that referenced this pull request Feb 11, 2023
**Related issue:**

 - Reverts #6716.
 - Closes #6782.
@swc-project swc-project locked as resolved and limited conversation to collaborators Mar 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants