Remove filename-unsafe but package-safe characters from bundle name #285
Conversation
src/utils/sanitizePackageName.js
Outdated
| @@ -0,0 +1,5 @@ | |||
| const unsafeRegex = /[/*]/g; | |||
There was a problem hiding this comment.
Let's go ahead and replace all invalid filename characters to avoid future issues:
/, \, <, >, :, ", |, ?, *
On windows, a filename also cannot end in .
There was a problem hiding this comment.
okay. if we want to create a more generic sanitizeFileName then we should use ‘sanitize-filename’ itself from npm, i think. as there are a bunch more illegal windows names.
i’ll do this tomorrow
There was a problem hiding this comment.
i prefer the the approach of trusting the package name to be a valid package name. I understand the want for a more generic approach, in case at some point you'd want to use this for things other than the package name.
But, if that was to happen, it would take no more work to include the login then that it would now, and trusting the package name isn't locking us in to anything that is complex or would mean a lot of rework should a future decision mean it needs changed.
But, i'm also fine with it and have done it and it's done now.
src/utils/sanitizePackageName.js
Outdated
| @@ -0,0 +1,5 @@ | |||
| const unsafeRegex = /[/*]/g; | |||
|
|
|||
| module.exports = function sanitizePackageName(name, replacement = '-') { | |||
There was a problem hiding this comment.
Could make this a more generic sanitizeFileName so that it can be used elsewhere if an issue presents itself.
src/utils/sanitizeFilename.js
Outdated
| const sanitizeFilename = require('sanitize-filename'); | ||
|
|
||
| module.exports = function sanitize(name, replacement = '-') { | ||
| return sanitizeFilename(unsafeRegex, {replacement}); |
There was a problem hiding this comment.
Is this un-tested?
unsafeRegex is no longer defined.
name is not passed to sanitizeFilename.
Doesn't look like this code would work as-is. The helper is probably unnecessary now, too. Can just use sanitize-filename directly.
There was a problem hiding this comment.
oh shoot, it was unpushed ! i'd amended the commit, and had not forced push.
i thought it preferable to keep the indirection in case more was ever desired of a sanitized package name than simple replacement (such as removing the namespace, or simply removing all non alpha-numerics). but that makes less sense now that it is not about package names but a generic filename sanitizer.
|
Would be great to add a test for this! Can you follow up with that @chee? Thanks!! |
|
@devongovett sure thing ! |
|
@devongovett i'm not sure where this test should go ! |
…285) * prettify the plugins test * replace package-safe but non-filesafe characters in package name * use the `sanitize-filename` module for entry name * remove sanitizeFilename indirection
…285) * prettify the plugins test * replace package-safe but non-filesafe characters in package name * use the `sanitize-filename` module for entry name * remove sanitizeFilename indirection
closes #283
Originally I was stripping off the namespace, and then hyphening everything else. Then I was using a module to strip out the filename unsafe characters.
but then, of course most filename-unsafe characters are already invalid package.json names, so now i'm only stripping out the remaining url/file unsafe characters which are still package.json safe. and that's only the
/in the namespace and the*allowed in old package names.so there