-
Notifications
You must be signed in to change notification settings - Fork 75
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
Remove unnecessary prepend of test with current folder #145
Conversation
@zinserjan could you please review this change and release |
@pkuczynski looks like all tests with relative entries are failing, so this isn't a solution. EDIT: Can you create a path utilitly for that and add some unit tests? For example a function like the following |
I noticed that tests are failing. I will have a look on Monday, ok? |
@zinserjan how would you like to bump |
@pkuczynski sending a PR there is the only reliable way to make sure that we are using a version that works with absolute paths. But before you can try this out on your project. |
I reverted the changes as you suggested, but 4 tests still fail:
Even if I updated local installation of globby to use glob 7.1. |
CI tests aren't failing, just the linting fails. For local development rebase your branch. I made several changes to make the tests working again :) |
This fixes an edge case where a file should be loaded from memory but it does also exist on real fs.
Indeed, I was removing a comma instead of adding it. Different linting style then the one I am used to :) |
Looks like the rebase rewrote the whole history. Can you fix this so that only your commits applied to the latest master? |
Codecov Report
@@ Coverage Diff @@
## master #145 +/- ##
==========================================
- Coverage 97% 96.99% -0.01%
==========================================
Files 28 28
Lines 767 765 -2
==========================================
- Hits 744 742 -2
Misses 23 23
Continue to review full report at Codecov.
|
I can confirm, that after re-base all the tests pass. It it did not require changing glob version in globby! Are we good to merge? |
PR rebased against latest |
No, bumping the version in globby is the only reliable way to make sure that the absolute path option is available. We cannot rely on npms module hoisting as it fails with npm@2 and it could also fail if another package needs glob < 7.1 and cause of the matching of glob > 7.0 in globby we will get 7.0 for glob :( That was the reason why I did #106, but I wasn't aware of the drawbacks... It was just the simplest solution... History is still messed up, but githubs squashing should fix this :D |
PR requested sindresorhus/globby#45 |
@zinserjan could you please comment in sindresorhus/globby#45? |
I think we should start to work an a different approach to speed things up and just ensure the absolute paths manually. Can you work on this on a different PR? So we can keep this open and merge it someday. I think something like this could work: import path from "path";
import normalizePath from "normalize-path"
function ensureAbsolutePath(path, basePath) {
return path.posix.isAbsolute(path) || path.win32.isAbsolute(path) ? path : path.join(basePath, path);
} |
Not sure if its worth the effort if undoing #106 make it work anyway. |
I added a temporary workaround to fix this issue (#160). We can continue the work on this PR when a new version of globby is available. |
@pkuczynski, would you like to rebase on |
Done |
You can squash in GH interface, right? |
Yep. The |
Sure. Done. Should I also remove it from
|
Good question. If I remember correctly, webpack emits just relative file paths. |
Ah I'm pretty sure right now, cause I was happy that we had this function already :) |
Good. So we are done with this PR, right? |
Yep. Thank you! 😉 |
Allow running individual tests from WebStorm environment. Works well also outside of WebStorm.
Fixes #115