Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept glob string for [entry files] ... #1205

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

chad-autry
Copy link

...in command "browserify [entry files] {OPTIONS}"
Windows does not auto expand globs on the command line

Creating the pull request to try and progress. It works locally for me, but an automated test case would be awesome. I can't tell how to make such a test case examining the existing suite.

This pull request is for #1170

…les] {OPTIONS}"

Windows does not auto expand globs on the command line
@terinjokes
Copy link
Contributor

I'm reviewing

@terinjokes
Copy link
Contributor

I think something similar the following would be prefered:

[].concat(opts.entries).filter(Boolean).reduce(function (globbed, input) {
  return globbed.concat(glob.sync(input));
}, []).forEach(function (file) {
  self.add(file, { basedir: opts.basedir });
});

I'd also prefer to isolate globs to being expanded in "bin/args.js", rather than in the library code.

@zertosh
Copy link
Member

zertosh commented May 6, 2015

Ah this would work great in watchify (see browserify/watchify#56). Don't forget to test it with the CLI not just the API 😛

@chad-autry
Copy link
Author

@terinjokes Yeah, I see how that fits more in with the current pattern. I just need to spend some more time to understand the current pattern a bit better. I'll try to at least get around copy pasting your suggested change and testing it.

@zertosh That issue does not appear to be the same thing. That issue is about NEW files. This issue is about globs simply not working at all (on Windows). Solving this problem in browserify did seem to solve the duplicate problem of globs not working with watchify when I pointed it at my edited repo.

@zertosh
Copy link
Member

zertosh commented May 6, 2015

@chad-autry My is to use the fact that entries maybe be globs to monitor for new files when implementing it in watchify - that's all.

@ghost
Copy link

ghost commented May 6, 2015

I don't know if this should be browserify's responsibility in the API. People can always call glob() themselves here.

Adding a glob.sync() call seems bad.

@chad-autry
Copy link
Author

@zertosh You wouldn't be able to since Linux systems would still expand the glob before it reached watchify

@substack I'll try to re-write it like the other other uses of glob in bin/args.js to not do glob.sync() For my use case I'm calling browserify from an npm script, was unable to call another program to substitute command parameters.

@zertosh
Copy link
Member

zertosh commented May 6, 2015

@chad-autry Not if it's quoted.

The use case I see in watchify is to monitor a directory of tests - where each test file needs to be an entry.

Watchify would look at b._options.entries to see if anything passes glob.hasMagic. If that's true, then watchify would setup another watcher with that glob, and on add/remove, it'll simply teardown the existing browserify instance and create a new one. This would only work with the CLI, since you have control there to create / destroy browserify instances.

Though, now that I think about it some more, this seems like a bad idea since it'll leave API users out in the cold.

…es] {OPTIONS}"

Windows does not auto expand globs on the command line
@chad-autry
Copy link
Author

I've made the logic more consistent with how browserify does things, and moved it over into "bin/args.js" as @terinjokes suggested.

@Lucas-C
Copy link

Lucas-C commented Feb 24, 2016

What is the status of this PR ? Can it be merged ?

@chad-autry
Copy link
Author

Requires re-review and hopefully acceptance by someone with permission to merge.

@nemoxps
Copy link

nemoxps commented Jun 24, 2017

@chad-autry Thank you so much for this patch! You saved my day :)
I can't believe this isn't merged yet. That is just poor. Probably going to use Webpack for further projects...

@mukururo
Copy link

Let's have the best globally moving projects.

@3cp
Copy link

3cp commented Jun 17, 2019

Please consider this improvement. It's very useful to run tests.

browserify 'test/**/*.spec.js' | tape-run

@goto-bus-stop goto-bus-stop self-requested a review June 17, 2019 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants