Added Multi extension support. #26

alansouzati · 2015-06-03T20:38:12Z

Hi, as agreed, I'm creating the PR again to add multi extension support!

alansouzati · 2015-06-03T20:38:48Z

This PR is intended to fix #24.

tkellen · 2015-06-03T20:51:04Z

@phated Any idea why the test suite is failing for babel.js?

alansouzati · 2015-06-03T20:54:45Z

It was actually my fault. The supportsExtension check should iterate over exts. If we don't find on the first one, move forward. In the babel case:

1 - .babel.js is not supported by default
2 - Then we move the to next extesion: .js, which is supported
3 - Result? It tries to interpret the file as raw js, when in fact it is babel.

tkellen · 2015-06-03T20:59:25Z

index.js

+
+  var supportsExtension = Object.keys(require.extensions).indexOf(exts[0]) !== -1;
+
+  if (supportsExtension) {
    return true;


You can't return here. Some extensions require further setup which is performed later.

I'm not sure I get your point. I believe this return was here before. This is basically the same check as used to be in the master branch.

This early return is not in master. See the configuration for babel.js here. The register method is handled by this, which is called here.

This early exit prevents any of that from happening and it also prevents exposing any errors with registering the module loader.

silkentrance · 2015-06-05T15:51:28Z

Since you are asking for multiple match groups, you are also implying that there is an upper limit to the number of possible extensions any one file can have. I presume that with at most three extensions the system should work fine, e.g.

const EXTRE = /(?:(?:\.[^.]+)?((?:\.[^.]+)?(\.[^.]+)))$/;

yields

> EXTRE.exec('testing.tmp.babel.js');
[ '.tmp.babel.js',
  '.babel.js',
  '.js',
  index: 7,
  input: 'testing.tmp.babel.js' ]

> EXTRE.exec('testing.babel.js')
[ '.babel.js',
  '.js',
  '.js',
  index: 7,
  input: 'testing.babel.js' ]

> EXTRE.exec('testing.js')
[ '.js',
  '.js',
  '.js',
  index: 7,
  input: 'testing.js' ]

tkellen · 2015-06-05T18:47:43Z

Just grab everything from the first dot, split on dot, and do this:

['.one', '.two', '.three'].map(function (item, idx, arr) {
  return arr.slice(idx).join('');
});

...that will give you ['.one.two.three', '.two.three', '.three']

alansouzati · 2015-06-05T18:48:53Z

Yeah, agreed. I believe this is the simplest solution. I will work on that.

On Fri, Jun 5, 2015 at 11:47 AM, Tyler Kellen notifications@github.com
wrote:

Just grab everything from the last dot, split on dot, and do this:

['.one', '.two', '.three'].map(function (item, idx, arr) {
return arr.slice(idx).join('');
});

...that will give you ['.one.two.three', '.two.three', '.three']

—
Reply to this email directly or view it on GitHub
#26 (comment).

Alan Souza
Software Engineer
Contact: +1 (408) 421 - 6341

alansouzati · 2015-06-05T19:29:13Z

lib/extension.js

    return;
  }
-  return extension[1];
+  return possibleExtensions.map(function (item, idx, arr) {
+    return arr.slice(idx).join('');


Thanks for the insight @tkellen!

great solution btw!

tkellen · 2015-06-09T16:17:21Z

@alansouzati Is this ready for review?

alansouzati · 2015-06-09T17:10:50Z

Sorry, let me just fix the typo in extension and it should be ready for review.

phated · 2015-06-09T17:12:14Z

Also, can you add to the test a path that includes a directory with a dot in it?

alansouzati · 2015-06-09T17:15:19Z

Sure @phated !

phated · 2015-06-09T18:02:29Z

index.js

-  if (Object.keys(require.extensions).indexOf(ext) !== -1) {
+  var exts = extension(filepath);
+
+  var supportsExtension = Object.keys(require.extensions).indexOf(exts[0]) !== -1;


i think we need to filter here and then check length, no?

Well, if you look at the validation I'm using only the first option exts[0] (e.g .babel.js). If we don't do this check we would treat .babel.js as .js files. You can check that by removing it and running the tests.

but shouldn't we check every extension to see if we support it? e.g. what if a user names something .babel.coffee for some reason

Yeah, that's a good point, but I'm not sure how to solve this unless we check semantics on the file, right?

I was thinking

var registered = exts.filter(function(ext){ return Object.keys(require.extensions).indexOf(ext) !== -1; }); if(exts.length === registered.length){ return true }

@tkellen am I thinking about this correctly?

actually, i don't think require.extensions will ever have something like .babel.js because node's loader doesn't work with that. hmmm, this is a tricky check

In the context of Liftoff, it was wrong of rechoir to throw on an unknown extension. An unknown extension could very well be standard JS (as is the case for @alansouzati). I've fixed this here: 62fb2a6

We still need to use some portion of this PR to support things like tmp.babel.js, though.

Cool. feel free to pick and choose whatever you need from this code. Then after that we can easily close the PR.

Fixed supportsExtension check. Fixed extensions for multi period. Fixing typo on the extension. Added tests for folders with dots.

phated · 2015-07-12T23:02:38Z

@alansouzati any chance you can tackle the remaining issue? I'd like to close some of the issues open related to this.

alansouzati · 2015-07-13T16:23:48Z

I'm uncertain how to tackle the remaining issue.. any suggestions?

phated · 2015-07-13T17:19:07Z

@alansouzati maybe that check can go away and we can check if the module that matches the extension is already in the require.cache - @tkellen thoughts? Someone opened this as a bug on gulp, so I'd like to get it wrapped up

dignifiedquire · 2016-06-23T21:13:01Z

@tkellen any chance of fixing this? I'm looking into using this module for karma config loading where I have files like karma.conf.babel.js or karma.conf.coffee or karma.config.js and this is a blocker for me.

This is currently blocked by gulpjs/rechoir#26 to work. When finished this will fix #2180,#1597,#1727

phated · 2019-01-02T21:56:30Z

Superseded by #37

alansouzati force-pushed the MULTI_EXTENSION_SUPPORT branch from 3380e69 to ed6cabb Compare June 3, 2015 20:52

tkellen reviewed Jun 3, 2015
View reviewed changes

alansouzati force-pushed the MULTI_EXTENSION_SUPPORT branch from ed6cabb to 74ff352 Compare June 5, 2015 19:28

alansouzati reviewed Jun 5, 2015
View reviewed changes

alansouzati force-pushed the MULTI_EXTENSION_SUPPORT branch from 74ff352 to f5cc3dd Compare June 9, 2015 17:14

phated reviewed Jun 9, 2015
View reviewed changes

Added Multi extension support.

3d0412c

Fixed supportsExtension check. Fixed extensions for multi period. Fixing typo on the extension. Added tests for folders with dots.

alansouzati force-pushed the MULTI_EXTENSION_SUPPORT branch from f5cc3dd to 3d0412c Compare June 9, 2015 20:30

tkellen mentioned this pull request Jun 15, 2015

throw new Error('No module loader found for "'+ext+'".'); #24

Closed

phated mentioned this pull request Jul 1, 2015

Invoking gulp --gulpfile gives error on OSX gulpjs/gulp#1129

Closed

dignifiedquire added a commit to karma-runner/karma that referenced this pull request Jun 23, 2016

wip: use rechoir and interpret for config loading

f331990

This is currently blocked by gulpjs/rechoir#26 to work. When finished this will fix #2180,#1597,#1727

dignifiedquire mentioned this pull request Jun 23, 2016

wip: use rechoir and interpret for config loading karma-runner/karma#2186

Closed

cspotcode mentioned this pull request Dec 19, 2016

Adds multi-extension support (update to #26) #34

Closed

phated mentioned this pull request Apr 11, 2018

add support for a gulpfile.babel.coffee configuration gulpjs/gulp-cli#160

Closed

phated mentioned this pull request Dec 28, 2018

Multi extension support #37

Merged

phated closed this Jan 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Multi extension support. #26

Added Multi extension support. #26

alansouzati commented Jun 3, 2015

alansouzati commented Jun 3, 2015

tkellen commented Jun 3, 2015

alansouzati commented Jun 3, 2015

tkellen Jun 3, 2015

alansouzati Jun 3, 2015

tkellen Jun 3, 2015

silkentrance commented Jun 5, 2015

tkellen commented Jun 5, 2015

alansouzati commented Jun 5, 2015

alansouzati Jun 5, 2015

silkentrance Jun 6, 2015

tkellen commented Jun 9, 2015

alansouzati commented Jun 9, 2015

phated commented Jun 9, 2015

alansouzati commented Jun 9, 2015

phated Jun 9, 2015

alansouzati Jun 9, 2015

phated Jun 9, 2015

alansouzati Jun 9, 2015

phated Jun 9, 2015

phated Jun 9, 2015

tkellen Jul 23, 2015

alansouzati Jul 23, 2015

phated commented Jul 12, 2015

alansouzati commented Jul 13, 2015

phated commented Jul 13, 2015

dignifiedquire commented Jun 23, 2016

phated commented Jan 2, 2019

Added Multi extension support. #26

Added Multi extension support. #26

Conversation

alansouzati commented Jun 3, 2015

alansouzati commented Jun 3, 2015

tkellen commented Jun 3, 2015

alansouzati commented Jun 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silkentrance commented Jun 5, 2015

tkellen commented Jun 5, 2015

alansouzati commented Jun 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkellen commented Jun 9, 2015

alansouzati commented Jun 9, 2015

phated commented Jun 9, 2015

alansouzati commented Jun 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phated commented Jul 12, 2015

alansouzati commented Jul 13, 2015

phated commented Jul 13, 2015

dignifiedquire commented Jun 23, 2016

phated commented Jan 2, 2019