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

fix crawler module resolver - #40 #36 #51

Merged
merged 3 commits into from
Dec 8, 2014
Merged

fix crawler module resolver - #40 #36 #51

merged 3 commits into from
Dec 8, 2014

Conversation

jasisk
Copy link
Member

@jasisk jasisk commented Aug 20, 2014

⚠️ This will require a major bump because it changes default behavior.

Fixes the directory crawler to properly ignore not requirable files (including dotfiles).

@totherik
Copy link
Member

Yes please.

function hasRequireHandler(file) {
var fileExt = path.extname(file);
var extensions = Object.keys(require.extensions);
return !!~extensions.indexOf(fileExt);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How's it exclude dotfiles?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require.extensions === .js or .node or .json (unless mutated).

The only time a dotfile will be included is if it's one of those files which, I would argue, is weird but appropriate behavior.

EDIT: as you mentioned in chat, .words.js will also be included. True. Do we want to exclude that behavior?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just seems odd. I'd expect auto-loading to ignore dotfiles, even if they're .thing.js. Less weird if explicit.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just like foo/* on the shell doesn't include foo/.*

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But parsing files in a dir in node does. Which behavior should we emulate (shell or node)? As mentioned, I could see either way so looking for feedback.

@jasisk
Copy link
Member Author

jasisk commented Dec 4, 2014

Updated. Will not traverse into dot directories and will ignore dot files. How we liking this behavior?

@aredridel
Copy link

Squash the three implementation commits together?

@aredridel
Copy link

Is there any good reason to make this backward compatible -- opt in, perhaps?

@aredridel
Copy link

Bonus points for partyscript!

@jasisk
Copy link
Member Author

jasisk commented Dec 4, 2014

partyscript is its own bonus.

@aredridel
Copy link

👍 unless it can be made backward compatible without making it horrible.

@totherik
Copy link
Member

totherik commented Dec 8, 2014

I think this is a fundamentally backward incompatible change. Again, unlikely to break things in the real world, but non-backward compatible nonetheless.

totherik added a commit that referenced this pull request Dec 8, 2014
fix crawler module resolver - #40 #36
@totherik totherik merged commit 30e88f8 into master Dec 8, 2014
@totherik totherik deleted the fix-crawler branch December 8, 2014 16:35
@jasisk
Copy link
Member Author

jasisk commented Dec 9, 2014

Closes #58

@jasisk
Copy link
Member Author

jasisk commented Mar 16, 2015

Closes #46

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