requireDir icon indicating copy to clipboard operation
requireDir copied to clipboard

Added check to see if folder is a valid node module

Open pezza3434 opened this issue 8 years ago • 12 comments

Added check to see if a folder is a node module as per require specs.

https://nodejs.org/api/modules.html#modules_folders_as_modules

@aseemk

pezza3434 avatar Nov 10 '15 17:11 pezza3434

@aseemk This is really useful, and the expected behaviour would it be possible to get this merged?

AlexMeah avatar Nov 12 '15 12:11 AlexMeah

Sorry I missed this; thanks for the nudge.

Interesting feature. One thing I'm unsure of is how this should relate to recurse. Currently, you're doing this as the default behavior. Is that best? Or is that counterintuitive / violates least surprise?

Also, would you mind adding this to the readme in the PR? Doesn't have to be perfect; it'd just be good to see how you would communicate this feature. Thanks! (And thanks for already adding tests.)

aseemk avatar Nov 12 '15 13:11 aseemk

Good idea btw and thanks for the PR!

aseemk avatar Nov 12 '15 13:11 aseemk

Hi @aseemk,

I've changed a lot of the small things that you mentioned, I also added to the readme.

I was also unsure about the recursion functionality. I decided to omit this functionality from the recursion option for a couple of reasons:

  1. It would be a major breaking change
  2. The recursion options goes through the entire directory, including sub-directories and so people may want the verbosity that it exposes.
  3. Looping through all the folders like that is not a 'node thing' and to resolve the folders as such would not be expected behaviour (for me anyway).

Perhaps going forwards there could be two recursion options, with and without verbosity. Verbosity would give you all the folders and contents while non verbosity would attempt to resolve each folder and treat it how we have in this pull request.

I look forward to your thoughts on the changes.

pezza3434 avatar Nov 12 '15 14:11 pezza3434

@aseemk Any updates on this? Thanks!

pezza3434 avatar Nov 19 '15 14:11 pezza3434

Sorry folks, I've been unusually busy with both work and life for a sustained period of time now. I keep meaning to get to these PRs (and other projects), but don't find myself with the time + energy too often.

One thing I've done with another project is to invite one or two other people to be collaborators in the project and help maintain it. Anyone here interested in that?

I'd just ask that you be mindful of keeping this project focused, and not let it get too complex. E.g. I'm not 100% sure it's a good idea to accept all of the currently open PRs, but I haven't looked into them fully or thought deeply about it.

Apologies again for the delay, and thank you for understanding. =)

aseemk avatar Jan 20 '16 13:01 aseemk

Hi there,

I'm sorry I haven't been timely in responding to these issues and PRs.

If you're interested in taking over the maintainer role for this project, please let me know.

More details: https://github.com/aseemk/requireDir/pull/31#issuecomment-255744987

Cheers, Aseem

aseemk avatar Oct 24 '16 13:10 aseemk

@pezza3434 Could you rebase this? I'll try to get this merged and published ASAP. Thanks for the contribution.

yocontra avatar Feb 07 '18 22:02 yocontra

It makes sense that this module follows node's cjs resolution algorithm by default. I'd like to argue that recurse should follow this resolution algorithm as well. I understand that this is a breaking change and that it would break a lot of code, including my own. Maybe it makes sense to add a new deep option that would recurse through folders but uses require's resolution algorithm?

Janpot avatar Feb 08 '18 09:02 Janpot

@Janpot I agree, I think this should be the default behavior (recurse or no recurse). As for breaking code, I'll publish it as a breaking release with a note in the changelog.

yocontra avatar Feb 08 '18 16:02 yocontra

ping @pezza3434

yocontra avatar Mar 09 '18 17:03 yocontra

I need this :)

thealjey avatar Jan 28 '19 14:01 thealjey