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: trim slashes from dest when flatten false; add support to ../ #47

Closed
wants to merge 26 commits into from

Conversation

brunoimbrizi
Copy link
Contributor

This PR addresses #46

  • it removes ../ from dir so dest reliably replaces the first folder in the path on .split('/')[0]
  • it trims slashes from the path so destDir doesn't end up in the root folder

const destDir =
flatten || (!flatten && !dir)
? dest
: // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
dir.replace(dir.split('/')[0]!, dest)
: destClean
Copy link
Owner

Choose a reason for hiding this comment

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

I guess this is same with the following code.

path.resolve(dest, path.normalize(path.join(dir, '../')))

Also would you add some test cases here?

export const testcases: Record<string, Testcase[]> = {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

path.resolve(dest, path.normalize(path.join(dir, '../')))

The result with this code is different from the one in the PR.
The goal is to target the build.outDir defined in vite.config. Your suggestion ended up copying the files somewhere else entirely. Is the code suggested in the PR not good? It does the job.

Also would you add some test cases here?

I'm sorry, I don't know how to do that.

Copy link
Owner

Choose a reason for hiding this comment

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

The result with this code is different from the one in the PR.

Ah, my bad I was misunderstanding. Please forget that.

I'm sorry, I don't know how to do that.

I added the tests. It seems your change breaks some behaviors.

@brunoimbrizi
Copy link
Contributor Author

My fork was behind, I tried to sync it, but the comparision with the main repo was showing changes in test files, which I didn't author and were probably out-of-date. I tried to fix it, which created unwanted extra commits and when trying to clean up the git history things got really messy. I'd rather close this PR now and perhaps submit a new one starting from a fresh fork.

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.

5 participants