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: #775 node_modules not ignored. #802

Merged
merged 2 commits into from
Mar 5, 2023
Merged

Conversation

annitya
Copy link
Contributor

@annitya annitya commented Feb 27, 2023

The logic has been taken form this post.

On windows the "node_modules"-entry in the excluded array is absolute and looks something like this: "c:\\users\\..". The path that is tested is in unix-format. The string-test startsWith fails to account for this.

I grabbed the code from StackOverflow, tested it directly by editing the compiled version of this plugin, modified it and added it here. It's late and I'm tired, so take a few extra looks at the logic. Our app would hang for about 30s, seemingly doing nothing after webpack was done compiling. It now starts instantly.

Copy link
Collaborator

@piotr-oles piotr-oles left a comment

Choose a reason for hiding this comment

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

Thanks for fixing that! Please add unit tests and extract the code so it's easier to understand the logic :)

Comment on lines 34 to 35
const relativePart = relative(excludedPath, path);
return relativePart && !relativePart.startsWith('..') && !isAbsolute(relativePart);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please extract this code to a separate function and file.

// src/utils/path/is-inside-another-path.ts
import { relative, isAbsolute } from 'path';

function isInsideAnotherPath(path: string, anotherPath: string): boolean {
  const relativePath = relative(anotherPath, path);
  
  return relativePath && !relativePath.startsWith('..') && !isAbsolute(relativePath);
}

export { isInsideAnotherPath };

and write some unit tests for this function :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see what I can do. I don't have much spare time at the moment, so I don't know when I'll be able to make the necessary adjustments.

@annitya
Copy link
Contributor Author

annitya commented Mar 3, 2023

'aight! I fixed it up a little.

Copy link
Collaborator

@piotr-oles piotr-oles left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, looks good 👍

src/utils/path/is-inside-another-path.ts Show resolved Hide resolved
@piotr-oles
Copy link
Collaborator

Looks like it doesn't work on linux and macos :)

@annitya
Copy link
Contributor Author

annitya commented Mar 4, 2023

Looks like it doesn't work on linux and macos :)

Can't even trust StackOverFlow? What's the world coming to... I'll take a look!

@piotr-oles
Copy link
Collaborator

Yeah, AFAIK, path behavior is platform specific unless you use path.win32 or path.posix

@annitya
Copy link
Contributor Author

annitya commented Mar 5, 2023

I modified the tests to use the platform-specific implementation. I reasoned that if you are matching against win32-paths when on Unix, something else entirely has gone majorly wrong.

@piotr-oles piotr-oles merged commit 83cbefb into TypeStrong:main Mar 5, 2023
@github-actions
Copy link

github-actions bot commented Mar 5, 2023

🎉 This PR is included in version 8.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@annitya
Copy link
Contributor Author

annitya commented Mar 6, 2023

Thanks for merging!

Tested the changes today. At first it didn't work, but then I found out why.
TypeScript-exclude supports glob-patterns, but the custom-matcher in fork-ts-checker-webpack-plugin does not. The logic is complex and while it's tempting to reuse, rather than recreate, the API is marked as internal.

On a positive note, globs never worked, so it's not worse than before. Thoughts?

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

Successfully merging this pull request may close these issues.

2 participants