Skip to content
This repository has been archived by the owner on Jan 25, 2020. It is now read-only.

Added CoffeeScript support #14

Merged
merged 2 commits into from
Feb 4, 2014
Merged

Conversation

simonexmachina
Copy link
Contributor

There's several ways that we could do this, but I'm not sure what your preference is so I've implemented how I think it should be supported: if .coffee is included in require.extensions then enrouten will load CoffeeScript files.

Other options include opt-in using a configuration option, or the ability to specify the set of extensions to be loaded in the config.

@totherik
Copy link
Member

I'm not in love with adding custom extension support here. All I see is maintenance burden. Would something like the following suffice?

function tryResolve(file) {
    try {
        // implicitly uses require.extensions to resolve known file types
        // json would be a noop since JSON files don't fulfill the required
        // exported function API
        file = file.replace(path.extname(file), '');
        require.resolve(file); 
        return true;
    } catch (err) {
       return false;
    }
}
// ... snip ...

} else if (stats.isFile() && tryResolve(file)) {

// ... snip ...

@simonexmachina
Copy link
Contributor Author

Yup, that's a good suggestion. I've done that an pushed.

@totherik
Copy link
Member

The sample code isn't very robust and should be improved before merging. For example, if file is something like /Users/me/src/git/myapp.js/routes/index.js this code will cause unexpected results.

Thanks for being so responsive @aexmachina.

@simonexmachina
Copy link
Contributor Author

Maybe I don't understand, but I'm not aware of any issue with using require
with an absolute path? I just did a test with /tmp/foo.js and
require('/tmp/foo') and it worked fine.

@lmarkus
Copy link
Contributor

lmarkus commented Jan 30, 2014

@totherik's comment was pointed toward the code behavior. In his example, .js appears twice in the filename (Once as the extension, once as part of the path). Doing a replace operation on the extension would alter the path as well.

Think about all the weird stuff that "clever" developers will come up with. 😆

@simonexmachina
Copy link
Contributor Author

Okay, gotcha. Good point, have pushed a more robust version.

@totherik
Copy link
Member

I was thinking some more along the lines of:

file.replace(new RegExp(path.extname(file) + '$'), '');

@simonexmachina
Copy link
Contributor Author

Yes, I considered that but then you'd need to escape the . and JavaScript doesn't have a RegExp.escape() function. Happy to do it that way though, have updated.

I also tested some assumptions about how path.extname() works, and it is sensible.

@totherik
Copy link
Member

Yeah, I don't want to be in the business of maintaining naive extension parsing regexes when I can rely on extname. It may not be a bad idea to have an explicit test case for this feature at this point. Consider this:

'use strict';

var path = require('path'),
    assert = require('assert');

function a(file) {
    var ext = path.extname(file);
    if (ext) {
        return file.replace(new RegExp('\\' + ext + '$'), '');
    }
    return file;
}

function b(file) {
    return file.replace(/\.[a-z]+$/i, '');
}

['', '.js', '.coffee', '.', '.pk12'].forEach(function (ext) {
    var expected, original;

    expected = path.join(__dirname, 'foo');
    original = expected + ext;

    assert.equal(a(original), expected, 'implementation a failed to parse ' + original);
    assert.equal(b(original), expected, 'implementation b failed to parse ' + original);
});

Function b fails for both . and .pk12. The rationale is not that I need to handle those cases, but that I don't need to worry about them or predict my inputs because I can consistently parse them. I KNOW the generated regex will work, while I can't be sure the basic regex will.

@simonexmachina
Copy link
Contributor Author

Yes I couldn't agree more. My latest commit (when I posted my last comment) didn't actually push, so you couldn't see it after my previous commit. Basically I have used extname() and have confirmed that it handles those edge cases. I added a test for them as well but removed it, I can re-add if you like.

@totherik
Copy link
Member

This looks good to me. One last question: you actually tested this with coffeescript, right? 😄 I don't have it configured to actually try out so I want to make sure at least one person has tested ensure this code change resolve this particular issue. Thanks, again, for your work on this @aexmachina

@lmarkus
Copy link
Contributor

lmarkus commented Jan 31, 2014

Just one last request from me. @aexmachina, would you mind contributing a simple example along the vein of what's in the krakenjs.com examples section? I'd be happy to link to it

@simonexmachina
Copy link
Contributor Author

Tested in CoffeeScript - yes, I've been using it with CoffeeScript.

Not sure what the example would be - it's exactly the same as JavaScript. I've updated README.md with some info. Feel free to suggest any other additions.

@totherik
Copy link
Member

totherik commented Feb 3, 2014

Either way, 👍 from me.

@lmarkus
Copy link
Contributor

lmarkus commented Feb 4, 2014

Kraken now supports CoffeeScript 👍
Thanks @aexmachina

lmarkus pushed a commit that referenced this pull request Feb 4, 2014
Added CoffeeScript support
@lmarkus lmarkus merged commit 3e7c3fc into krakenjs:master Feb 4, 2014
@simonexmachina
Copy link
Contributor Author

Go team!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants