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

Nested src dir shows up in out dir #108 #123

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kimhongsu
Copy link

@kimhongsu kimhongsu commented Mar 12, 2022

  • Relative path support in CLI

    • Let's convert the user input paths to the absolute paths
    • As-Is: We had to use $PWD to point out the current directory
    • To-Be we can use the relative path. Dot notation is supported.
      swc ../../src --out-dir dist
    • filenames and outDir which are passed by arguments will be checked and converted to the absolute paths.
      const filenamesAbsolutePath = absolutePath(filenames, cwd);
      const outDirAbsolutePath = slash(resolve(cwd, outDir));
    • This will resolve the issue #2210
  • Source directory layout will be maintained to the output directory. So Nested src dir shows up in out dir #108 will be resolved.

    • As-Is: Source directory information which is typed in by the user is vanished on the process. But this information is important to maintain the output directory layout. When user typed in the source directory, it is substituted to all filenames by globSources(). We had only filenames. So we couldn't reorganize the output directory layout without the source directory information.
    • To-Be: We record the user input part in filename. I added File interface for this purpose. File.sourceLength is the index of the user input's end(source directory length) in filename. I add Number type in File interface to minimize the memory consumption and the fast process using slice() function.
    • From the absolute path of file, we eliminate the source directory part. Then we have a relative path. We can combine this relative path to the absolute path of destination that is specified by '--out-dir' argument. Now we can reconstruct the output directory layout.
    • I tried to not to change any CLI interface which is supported now.

And I think this will resolve the issue #2092 too.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


kimhongsu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@kimhongsu
Copy link
Author

I changed the function signature so I couldn't make this pull request pass the test.

@kdy1
Copy link
Member

kdy1 commented Mar 12, 2022

Can you update the referenced functions, too?

cc @kwonoj @alexander-akait
Can you take a look?
I'm not sure if this is what we want to support, and api can change in v2

@kimhongsu
Copy link
Author

I should have tested yarn types. I made a data type mismatch on watchCompilation() function and test case of sources.test.ts. I will figure it out how to solve it and fix this problem.

@kwonoj
Copy link
Member

kwonoj commented Mar 12, 2022

First of all, I would like to suggest consolidate effort to new CLI (swc-project/swc#3859 / https://github.com/swc-project/swc/blob/main/crates/swc_cli/src/commands/compile.rs)

Secondly, I need to think about interfaces but the way I'm thinking is let user choose to specific CWD to calculate path to src / dest, instead of trying to calculate between src / dest. I haven't completely go through all possible cases though. Generally, I'm bit against let swc trying to traverse up to current working dir, but that's probably something we have to support anyway.

@alexander-akait
Copy link

Secondly, I need to think about interfaces but the way I'm thinking is let user choose to specific CWD to calculate path to src / dest, instead of trying to calculate between src / dest. I haven't completely go through all possible cases though. Generally, I'm bit against let swc trying to traverse up to current working dir, but that's probably something we have to support anyway.

@kwonoj Agree, the --cwd option is the best solution here to avoid ../../.. (most known tools support it), maybe even we should support it in .swcrc configuration file.

Anyway --cwd is $PWD by default (like in other tools), so swc ../../src --out-dir dist should work and I think we should support it

@kimhongsu
Copy link
Author

kimhongsu commented Mar 13, 2022

First of all, I would like to suggest consolidate effort to new CLI (swc-project/swc#3859 / https://github.com/swc-project/swc/blob/main/crates/swc_cli/src/commands/compile.rs)

I wish I could write RUST. My problem is that I don't know RUST.

Secondly, I need to think about interfaces but the way I'm thinking is let user choose to specific CWD to calculate path to src / dest, instead of trying to calculate between src / dest. I haven't completely go through all possible cases though. Generally, I'm bit against let swc trying to traverse up to current working dir, but that's probably something we have to support anyway.

I don't know how CWD should work exactly but...
I think my implementation supports --cwd option implicitly. Please let me know If I misunderstood the concept of CWD what you guys were talking about.

Now I understood --cwd option.

By the way the command below will make an user choose CWD It will maintain the output layout as the user expected in my implementation.
./bin/swc.js /home/ubuntu/workspace/myproject --out-dir /home/ubuntu/Downloads/dist


And I'm still stuck into the argument type mismatch in watchCompilation(). I'm shameful.
chokidar library interface doesn't fit with my implementation. I will try to solve it.

@kimhongsu
Copy link
Author

I resolved the type mismatch error. And I appended --cwd option too.

There is a failure on integration test. I'm not sure this error is related to my code. If so, please let me know.

Error: Error occurred while loading config file at /home/runner/work/cli/cli/integration-tests/three.js/spack.config.js: Error [ERR_REQUIRE_ESM]: require() of ES Module /home/runner/work/cli/cli/integration-tests/three.js/spack.config.js from /home/runner/work/cli/cli/node_modules/@swc/core/spack.js not supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants