Fix regexp to not find extensions in the middle of a module name #11
Conversation
| @@ -25,7 +25,7 @@ describe('babel-plugin-css-modules-transform', () => { | |||
| } | |||
|
|
|||
| function readExpected(path) { | |||
| return readFileSync(resolve(__dirname, path), 'utf8'); | |||
| return readFileSync(resolve(__dirname, path), 'utf8').trim(); | |||
There was a problem hiding this comment.
Used this so that we remove trailing new lines from the expected files. Some (like me) have the editors configured so that the new line is automatically inserted, so figured it'd be good to just ignore that instead of having an exception for these files. If this is something you don't want, I can remove it and remove the new line in the expected file.
There was a problem hiding this comment.
I ran into this too... That said, I'd say it merits a comment to explain why we'd want to trim a file's contents when used as a text fixture.
There was a problem hiding this comment.
Happy to do that, if @michalkvasnicak decides it's worth keeping
There was a problem hiding this comment.
Do you think this is worth adding? Maybe in another PR?
There was a problem hiding this comment.
@leoasis what do you mean? Adding the regex for query params mentioned by @jvivs (#11 (comment))? Because trimming is already merged so I don't understand this comment. :)
There was a problem hiding this comment.
Just meant adding a comment to explain this, as @jvivs suggested
There was a problem hiding this comment.
I see :) Ok, can you add it in a new PR please? :)
| const extensionsPatern = extensions.join('|').replace('.', '\.'); | ||
| return new RegExp(`(${extensionsPatern})`, 'i'); | ||
| const extensionsPatern = extensions.join('|').replace(/\./g, '\\\.'); | ||
| return new RegExp(`(${extensionsPatern})$`, 'i'); |
There was a problem hiding this comment.
If we're anchoring the match at the end of a stylesheet path (which I think we should), we might want to take into account possible url parameters too... thoughts?
We could use something like this instead:
return new RegExp(`(${extensionsPatern})(\?.*)?$`, 'i');There was a problem hiding this comment.
I am okay with that :) Please create a PR with tests.
|
Great! thanks @michalkvasnicak |
This PR fixes the Regexp used to filter modules with the supported extensions. Previously it was incorrectly finding modules that contained the extension in the middle of the name. This was both because we were not searching for the last characters in the module (using
$) and also because it was not escaping the.caracter properly.Added some more cases to the tests for this fix.
Oh and by the way, thanks for creating this module! Very helpful!