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 Webpack.require() when file is outside cwd #37

Merged
merged 9 commits into from
Jun 3, 2018

Conversation

kLabz
Copy link
Contributor

@kLabz kLabz commented May 23, 2018

I am using a strange environment where the main sources are outside the cwd.

When requiring with Webpack.require(), I end up with paths like .//my/absolute/path/to/src/file.scss which don't work.

I tested this version with all my projects, it worked fine. I don't know about windows nor how far the test suite goes in testing this kind of things, though.

Now including changes from #36 too:

Now parsing both hxml and options.extra with yargs-parser's tokenize function, that sanitizes command-line arguments.

It is also now properly handling --macro (and other arguments) quotes, which is not the case atm (doubles the quotes if we have quotes in our hxml already, which solved #28 but broke the opposite case).

@@ -126,7 +128,7 @@ class Webpack {
}

if (directory != '') {
return './${directory}/${file}';
return '${directory}/${file}';
}
if (file.startsWith('.')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't this be '..' instead of '.'?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need unit tests here before changing the behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I don't have too much time for that atm though =/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah this is a bit sensitive so I'll try to write some

kLabz added a commit to kLabz/webpack-haxe-loader that referenced this pull request May 26, 2018
@kLabz kLabz force-pushed the fix/require-absolute-directory branch 5 times, most recently from d2dc156 to 2b3dd9d Compare May 29, 2018 11:23
assert(rebaseRelativePath('src/a/b', './res') == './src/a/b/res');
assert(rebaseRelativePath('src/a/b', '../res') == './src/a/res');
assert(rebaseRelativePath('src/a/b', '../../res') == './src/res');
assert(rebaseRelativePath('src', './res') == 'src/res');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the cases should be modified to be like rebaseRelativePath('./src', './res'), and adding some dummy absolute paths, because that's what comes our the relativePath function now.

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 started and yep, needs more work on both the tests and the implementation. I'll try to update this week-end (or maybe before if I find some time).

@kLabz kLabz force-pushed the fix/require-absolute-directory branch from 2b3dd9d to bd1afa8 Compare May 29, 2018 13:59
@kLabz kLabz force-pushed the fix/require-absolute-directory branch 3 times, most recently from a533235 to add97a3 Compare June 3, 2018 08:20
@kLabz kLabz force-pushed the fix/require-absolute-directory branch from add97a3 to 71aa41b Compare June 3, 2018 08:22
@elsassph elsassph merged commit 55066e2 into jasononeil:master Jun 3, 2018
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.

None yet

2 participants