-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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 detection of parent directory in ModuleScopePlugin #2405
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks! If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact [email protected] if you have any questions. |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
LGTM, can you show me a sample path of this though? Curious to see the problem as I'm not really visualizing it too well. |
Of course.
|
Oh okay, I didn't realize a webpack plugin would dirty your src directory. Makes perfect sense now, thanks! :) |
I'm not expert enough to confirm nor deny your assertion ;) |
@@ -49,7 +49,7 @@ class ModuleScopePlugin { | |||
) | |||
); | |||
// Error if in a parent directory of src/ | |||
if (requestRelative[0] === '.') { | |||
if (requestRelative.startsWith('../')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably use path.sep
instead of /
for cross platform reasons. 😄
You could also be explicit and just add a second case for \
.
Should we change the check above, too? I'm not sure if people use dot directories often.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the windows "reminder"! I added a second case to match the style above in the code.
I also fixed the first detection (different situation though).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this will work on Windows machines. Please see my comment.
Add windows specific path and extend to issuer path
Great, thanks! |
* master: Add WebStorm >2017 launchEditor Support (facebook#2414) docs: update `jest-enzyme` section (facebook#2392) Fix detection of parent directory in ModuleScopePlugin (facebook#2405) Added cache clear to e2e scripts (facebook#2400) Fix kill command in e2e-kitchensink.sh cleanup (facebook#2397) Revert "Catch "No tests found" during CI" (facebook#2390) Fix wrong path expansion in end-to-end test (facebook#2388) Suggest just "yarn build" (facebook#2385) Catch "No tests found" during CI (facebook#2387) Publish Add changelog for 1.0.7 (facebook#2384) Update webpack to 2.6.1 (facebook#2383) Consistently set environment variables (facebook#2382) Disable comparisons feature in uglify compression in production (facebook#2379) Removed the overriding of reduce_vars to false since webpack v2.6.0 included the fixed for Uglify bug (facebook#2351)
* Fix detection of parent directory * Correct parent directory detection fix Add windows specific path and extend to issuer path
* Fix detection of parent directory * Correct parent directory detection fix Add windows specific path and extend to issuer path
This PR fixes #2404.