Multi extension support #37
Conversation
Fixed supportsExtension check. Fixed extensions for multi period. Fixing typo on the extension. Added tests for folders with dots.
|
I'd also like to get this landed before I rebase the project to inline with the other gulp repos. |
| }); | ||
| } | ||
|
|
||
| if(Object.keys(require.extensions).indexOf(usedExtension) !== -1) { |
There was a problem hiding this comment.
Boolean(require.extensions[ext]) is faster.
Or is it possible that a value of an existing key is falsy?
There was a problem hiding this comment.
Sorry, it is possible that usedExtension is undefined and require.extensions[usedExtension] becomes require.extensions['undefined'].
Cancel the above comment, or move this line after checking config is not nullish if you will modify.
There was a problem hiding this comment.
I think it's fine to keep like this.
lib/extension.js
Outdated
| @@ -1,11 +1,19 @@ | |||
| const path = require('path'); | |||
|
|
|||
| const EXTRE = /^[.]?[^.]+([.].*)$/; | |||
| const EXTRE = /\.[^.]+/g; | |||
| const LONGEXTRE = /^[.]?[^.]+([.].+)$/; | |||
There was a problem hiding this comment.
/^[.]?[^.]+([.].+)$/.exec('aaa.js.') returns [ 'aaa.js.', '.js.', ...].
And '.js.'.match(EXTRE) returns '.js'.
Is it correct that const LONGEXTRE = /^[.]?[^.]+([.][^.]+)$/;?
There was a problem hiding this comment.
@sttk yeah, I think your regexp is better than mine. Let me come up with some tests.
There was a problem hiding this comment.
@sttk actually, it seems the LONGEXTRE needs to capture from the first . to the end of line, including future .s.
There was a problem hiding this comment.
I've made the above changes that I suggested and added a test. Let me know what you think.
There was a problem hiding this comment.
@sttk your above RegExp is pretty good but it completely rejects ANY path that has double dots. I think that is incorrect.
There was a problem hiding this comment.
I think some of these should return proper extensions, even though there's weird double-dot syntax inside the basename.
Sure. We should change EXTRE to support double-dot.
There was a problem hiding this comment.
@phated Which should we adopt for EXTRE?
'.bbb.ccc..js'.match(/\.+[^.]+/g); // => [ '.bbb', '.ccc', '..js' ]'.bbb.ccc..js'.match(/\.[^.]*/g); // => [ '.bbb', '.ccc', '.', '.js' ]
There was a problem hiding this comment.
@sttk I think 2 is correct.
There was a problem hiding this comment.
@sttk I just pushed changes that use your suggestion (2) and included tests. Let me know if you think that solves the issue.
|
@sttk I'm hoping you have a chance to review the small updates soon, I'd like to get this wrapped up before Monday. |
|
Thanks for reviewing @sttk!!! |
|
@phated Great work! |
@sttk I've had this as a WIP for quite awhile but didn't submit it because the tests were very out-of-date and I wanted to get things passing before I sent it.
Can you review and determine if this is a proper solution for #26 and #34? I think I've properly accounted for all the weirdness around multiple extensions but another set of eyes is needed.