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 #231 allow square brackets in path #264

Merged
merged 7 commits into from
Jun 1, 2018

Conversation

loveky
Copy link
Contributor

@loveky loveky commented May 23, 2018

fixes #231 fixes #220

@jsf-clabot
Copy link

jsf-clabot commented May 23, 2018

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Need tests

@loveky
Copy link
Contributor Author

loveky commented May 24, 2018

@evilebottnawi

I have a problem when setting up development environment on Windows. Because the file name/dir name under tests/helpers containes ? * which is not allow on Windows. git clone/git reset just fails. same with #220

image

Currently what I have done:

  1. clone the reproduce repo in Won't copy files when there are square brackets [ ] in the path #231; npm install; modify copy-webpack-plugin under node_modules, verify it solves the issue.
  2. on mac clone copy-webpack-plugin repo. apply the same change, run test to make sure it didn't broken on mac system.

any suggestion?

@alexander-akait
Copy link
Member

alexander-akait commented May 24, 2018

@loveky can you fix it in this PR? Just disable this test if you in window

@loveky
Copy link
Contributor Author

loveky commented May 25, 2018

got it. will work on that

@@ -8,7 +8,8 @@ export default function escape(context, from) {
// Handles special characters in paths
const absoluteContext = path.resolve(context)
.replace(/\\/, '/')
.replace(/[\*|\?|\!|\(|\)|\[|\]|\{|\}]/g, (substring) => `\\${substring}`);
.replace(/[\*|\?|\!|\(|\)|\{|\}]/g, (substring) => `\\${substring}`)
.replace(/\[/g, '[[]');
Copy link
Member

Choose a reason for hiding this comment

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

Looks strange. Can you describe why past solution doesn't work?

@loveky
Copy link
Contributor Author

loveky commented May 27, 2018

@evilebottnawi

the reason why I modified escape.js is that minimatch will replace all \ in path to /。This leads to a wrong directory hierarchy since it translate \[special directory\] to /[special directory/]. So I use another way to escape special characters by putting every character into a [] which makes the character lost its special meaning.

In the updated commits, I also move the creation of special helper directory to a script. The scrip will create directories/files based the system developers working on. This should also resolve #220

const dir = path.dirname(file);
mkdirp.sync(path.join(baseDir, dir));
fs.writeFileSync(path.join(baseDir, file), specialFiles[originFile]);
});
Copy link
Member

Choose a reason for hiding this comment

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

Add \n at end file

Copy link
Member

Choose a reason for hiding this comment

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

@loveky just one small fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which file should I add \n? createSpecialDirectory.js itself? or the files created by it? 🤣

Copy link
Member

Choose a reason for hiding this comment

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

@loveky in this file 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 😃

tests/index.js Outdated
@@ -15,6 +15,8 @@ import cacache from 'cacache';
import isGzip from 'is-gzip';
import zlib from 'zlib';

import removeIllegalCharacterForWindows from '../scripts/removeIllegalCharacterForWindows';
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be in src/utils instead of scripts/ otherwise it won't get published.

Copy link
Member

Choose a reason for hiding this comment

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

@loveky yep, move this to tests/utils

@filipesilva
Copy link
Contributor

Tried a build from this PR on my windows machine with a directory using parenthesis and the assets were copied.

@filipesilva
Copy link
Contributor

The lack of new tests might lead to regressions on special character support though.

@loveky
Copy link
Contributor Author

loveky commented May 31, 2018

Hi @filipesilva , the square brackets is already in the tests. Should I add more tests? any suggestion?

@filipesilva
Copy link
Contributor

I did not realize that, sorry for the confusion!

@alexander-akait
Copy link
Member

@filipesilva can we merge this, for me LGTM, but i don't have windows near

tests/index.js Outdated
@@ -15,6 +15,8 @@ import cacache from 'cacache';
import isGzip from 'is-gzip';
import zlib from 'zlib';

import removeIllegalCharacterForWindows from '../scripts/removeIllegalCharacterForWindows';
Copy link
Member

Choose a reason for hiding this comment

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

@loveky yep, move this to tests/utils

@@ -984,7 +993,7 @@ describe('apply function', () => {
'nested/nestedfile.txt'
],
patterns: [{
from: '[special?directory]'
from: (path.sep === '/' ? '[special?directory]' : '[specialdirectory]')
Copy link
Contributor

Choose a reason for hiding this comment

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

The path.sep conditionals are fairly non-obvious as to their purpose. Separate windows only tests would probably make more sense and simplify maintenance moving forward.

@evilebottnawi A Windows CI instance would probably be very useful for this plugin since windows pathing is vastly different than linux/MacOS/etc.

Copy link
Member

Choose a reason for hiding this comment

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

@clydin we already use appveyor for many plugins, need merge webpack-default here, will be done in near future

tests/index.js Outdated
@@ -1390,7 +1399,7 @@ describe('apply function', () => {
'noextension'
],
options: {
ignore: ['directory/**/*', '\\[special\\?directory\\]/**/*']
ignore: ['directory/**/*', (path.sep === '/' ? '\\[special\\?directory\\]/**/*' : '[[]specialdirectory]/**/*')]
Copy link
Contributor

@clydin clydin May 31, 2018

Choose a reason for hiding this comment

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

The escape sequences should be consistent and there should really be a test for both escaping techniques (where appropriate) as they are both accepted.

const path = require('path');

module.exports = function (string) {
return path.sep === '/' ? string : string.replace(/[*?"<>|]/g, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

If those are illegal characters on windows only then a check for windows should be used instead of path.sep which does not guarantee windows. (process.platform === 'win32'?)

const specialFiles = {
'[special?directory]/nested/nestedfile.txt': '',
'[special?directory]/(special-*file).txt': 'special',
'[special?directory]/directoryfile.txt': 'new'
Copy link
Contributor

Choose a reason for hiding this comment

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

a test with ! in the file/path should be added to ensure that [!] works as intended.

Copy link
Member

Choose a reason for hiding this comment

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

@loveky yep, add test

@@ -8,7 +8,7 @@ export default function escape(context, from) {
// Handles special characters in paths
const absoluteContext = path.resolve(context)
.replace(/\\/, '/')
.replace(/[\*|\?|\!|\(|\)|\[|\]|\{|\}]/g, (substring) => `\\${substring}`);
.replace(/[\*|\?|\!|\(|\)|\[|\{|\}]/g, (substring) => `[${substring}]`);
Copy link
Contributor

Choose a reason for hiding this comment

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

The closing brace should probably stay for consistency (i.e., makes for a simple rule of escape all special characters).

@@ -8,7 +8,7 @@ export default function escape(context, from) {
// Handles special characters in paths
const absoluteContext = path.resolve(context)
.replace(/\\/, '/')
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this replacement be removed if the character class escaping method is being used?

Also, on MacOS, this r\!4 is a valid directory name.

@filipesilva
Copy link
Contributor

I can say that in my windows machine the test suite in npm test passed. Would you like me to check something else on my machine?

@alexander-akait
Copy link
Member

@filipesilva not, just this PR

@loveky
Copy link
Contributor Author

loveky commented Jun 1, 2018

@evilebottnawi @clydin @filipesilva I have updated code and added some new tests.

@alexander-akait
Copy link
Member

wait @filipesilva approve

@filipesilva
Copy link
Contributor

Oh sorry, didn't mean to keep you waiting!

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Thanks to everyone.

@alexander-akait alexander-akait merged commit 3ef5b6c into webpack-contrib:master Jun 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Won't copy files when there are square brackets [ ] in the path Cannot clone repo on windows
5 participants