Add support for configurable file extensions #163
Conversation
| appendTransform(handleJs) | ||
|
|
||
| this.extensions.forEach(function (ext) { | ||
| require.extensions[ext] = require.extensions['.js'] |
There was a problem hiding this comment.
Maybe we should use default-require-extensions here? Just in case require.extensions['.js'] has been messed with already by some other module?
There was a problem hiding this comment.
I've updated to use this.
|
The new extensions are pointed to |
| @@ -212,7 +217,14 @@ NYC.prototype._maybeInstrumentSource = function (code, filename, relFile) { | |||
| return null | |||
| } | |||
|
|
|||
| return this.transform(code, {filename: filename, relFile: relFile}) | |||
| var ext = path.extname(filename) | |||
There was a problem hiding this comment.
Note that extname only returns the last extension. Looping through the registered transforms to see if they match the filename would let us support .foo.bar.
There was a problem hiding this comment.
I've updated to use the Array filter function to find matching extensions.
|
@lloydcotten would it be possible to add a test for this functionality? I differ to @jamestalmage and @novemberborn's other comments -- they've become the subject matter experts on the codebases that facilitate the extension overrides. |
|
@novemberborn This would not support CoffeeScript or TypeScript. We would have to somehow allow custom hooks to be added. Wouldn't that work the same way as it already does for Babel anyway? |
|
@bcoe I took some time today to try to figure out the best way to add some tests around this. I've taken a stab at it. Not sure if it's accomplished the best way possible, but the functionality is covered and passes. Any feedback is welcome. I'll be glad to try to refactor anything. Once every one is OK with it, I'll squash the commits. I'm removing the [WIP] tag. |
|
lloydcotten
changed the title
| @@ -53,7 +54,12 @@ function NYC (opts) { | |||
| // require extensions can be provided as config in package.json. | |||
| this.require = arrify(config.require || opts.require) | |||
|
|
|||
| this.transform = this._createTransform() | |||
| this.extensions = arrify(config.extensions || opts.extension) | |||
There was a problem hiding this comment.
This should access config.extension I think. Like --require results in opts.require and config.require, so should --extension.
There was a problem hiding this comment.
Perhaps add .js onto this.extensions. That way you can use it in _maybeInstrumentSource(). Could also map it to ensure all extensions are lowercased.
I'm not sure really. Not the end of the world if that doesn't work right away.
Tests look pretty good! Should test with multiple extensions since there is looping logic. The case insensitive matching needs covering as well. |
|
@novemberborn I updated the PR with some refactoring based on your comments. When I hear your feedback on my question about the tests, I'll modify the tests as necessary and add one to cover case insensitive matching. |
| @@ -53,7 +54,14 @@ function NYC (opts) { | |||
| // require extensions can be provided as config in package.json. | |||
| this.require = arrify(config.require || opts.require) | |||
|
|
|||
| this.transform = this._createTransform() | |||
| this.extensions = arrify(config.extension || opts.extension).concat(['.js']).map(function (ext) { | |||
There was a problem hiding this comment.
You can concat with strings. In other words concat('.js') is sufficient.
There was a problem hiding this comment.
Ha - learn something new every day. I did not know that.
|
@lloydcotten nice work so far! We can clean up the main |
| @@ -12,7 +12,8 @@ | |||
| "exclude": [ | |||
| "**/blarg", | |||
| "**/blerg" | |||
| ] | |||
| ], | |||
| "extension": [".es6", ".foo.BAR"] | |||
There was a problem hiding this comment.
Until it is easier to use opts over package.json config, I mixed the casing here a bit to test case-insensitive extension matching.
|
@lloydcotten thanks for the hard work, I've been swamped this week but will get a release out Saturday. |
|
@lloydcotten this is great, thanks! Looks like your PR now contains some duplicate commits that are already in |
|
@bco, @novemberborn you're welcome. Thanks for the cooperation and feedback on the PR. Not sure why the duplicate commits - must've goofed with my squash somehow. I'll take a look. |
|
OK - I've cleaned up the commit history. Should be good to go now. Let me know if you need anything else on this PR. |
Add support for configurable file extensions
|
@lloydcotten thanks for the wonderful contribution \o/ |
This adds support for allowing files with extensions other than '.js' to be covered. It can be configured through package.json config, or command line args.
As per my comment in issue #161 (comment), there aren't any additional tests yet. If you feel some additional tests to cover this functionality would be helpful, I'd appreciate some guidance as to how you think they could be done (I'm not quite used to the way the existing tests are done). Even just pointing me to existing tests that I could pattern after, would be great.
I'd be glad to address any other comments also.