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

based on various discussions I've switched to require.main.filename #3

Merged
merged 4 commits into from
Feb 6, 2016

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Jan 30, 2016

Based on discussions with @sindresorhus, @isaacs, and @iarna, I've switched to using:

require.main.filename

I've thoroughly tested this approach with the following testing module:

https://www.npmjs.com/package/yargs-testing-module

And the following branch of yargs, which has integration tests for symlinks:

yargs/yargs#356

Using the two approaches outlined above, I've tested the following on npm 2.x and npm 3.x:

  • global installs.
  • running locally.
  • symlinks.
  • and nested dependencies.

Sorry for keeping this conversation going so long, I really want to be careful to get this right (yargs@4.x needs to be an improvement not a step backwards).

Reviewers: @sindresorhus, @jameswomack, @nexdrew

@nexdrew
Copy link
Member

nexdrew commented Jan 30, 2016

This looks good on first blush, but is require.main dependent on how the Node process is initiated/executed? Is the behavior consistent across OS X, Linux, and Windows? I certainly defer to the experts - it's just that I read a couple things that has me concerned - but I'm quite confident in your testing skills; so if you're satisfied, I'm satisfied.

@jameswomack
Copy link

Given the couple edge cases like Unitech/pm2#66 & iisnode on Microsoft Azure, should we do one or more of the following
a) point out the known edge cases in the readme or code comments
b) say iisnode is not supported
c) use an existing micromodule that handles Azure-like environments where the entry point is intercepted?

Does NPM have testing infrastructure for trying out modules such as this w/ iisnode on Azure?

@bcoe
Copy link
Member Author

bcoe commented Jan 31, 2016

@jameswomack it seems like there's a fairly easy workaround on iisnode:

{id: '.',
  exports: {},
  parent: null,
  filename: 'D:\\Program Files (x86)\\iisnode\\interceptor.js',
  loaded: false,
  children: 
   [ { id: 'D:\\home\\site\\wwwroot\\server.js',
       exports: [Object],
       parent: [Circular],
       filename: 'D:\\home\\site\\wwwroot\\server.js',
       loaded: false,
       children: [Object],
       paths: [Object] } ],
  paths: 
   [ 'D:\\Program Files (x86)\\iisnode\\node_modules',
     'D:\\Program Files (x86)\\node_modules',
     'D:\\node_modules' ] }

We could use the children[0].filename if we see the iisnode bin. seems like a good thing to pull into a helper module, unless you know of one already?

@bcoe
Copy link
Member Author

bcoe commented Jan 31, 2016

@jameswomack @nexdrew testing with pm2, it looks like newer versions do maintain a correct require.path.filename:

pm2 output:

yargs-testing-module-0 (out): Module {
yargs-testing-module-0 (out):   id: '.',
yargs-testing-module-0 (out):   exports: {},
yargs-testing-module-0 (out):   parent: null,
yargs-testing-module-0 (out):   filename: '/Users/benjamincoe/bcoe/yargs-testing-module/bin/yargs-testing-module.js',
yargs-testing-module-0 (out):   loaded: false,
yargs-testing-module-0 (out):   children: 
yargs-testing-module-0 (out):    [ Module {
yargs-testing-module-0 (out):        id: '/Users/benjamincoe/bcoe/yargs-testing-module/node_modules/yargs/index.js',
yargs-testing-module-0 (out):        exports: [Object],
yargs-testing-module-0 (out):        parent: [Circular],
yargs-testing-module-0 (out):        filename: '/Users/benjamincoe/bcoe/yargs-testing-module/node_modules/yargs/index.js',
yargs-testing-module-0 (out):        loaded: true,
yargs-testing-module-0 (out):        children: [Object],
yargs-testing-module-0 (out):        paths: [Object] } ],
yargs-testing-module-0 (out):   paths: 
yargs-testing-module-0 (out):    [ '/Users/benjamincoe/bcoe/yargs-testing-module/bin/node_modules',
yargs-testing-module-0 (out):      '/Users/benjamincoe/bcoe/yargs-testing-module/node_modules',
yargs-testing-module-0 (out):      '/Users/benjamincoe/bcoe/node_modules',
yargs-testing-module-0 (out):      '/Users/benjamincoe/node_modules',
yargs-testing-module-0 (out):      '/Users/node_modules',
yargs-testing-module-0 (out):      '/node_modules' ] }

I think writing a shim for iisnode would be smart, that falls back to children[0], and we could make this module extensible such that we can add other edge-cases that we find.

My gut is that this would cover the majority of our use-cases, and we could address others in issues as they come up (updating the tiny shim module). how do you feel about this @nexdrew?

@nexdrew
Copy link
Member

nexdrew commented Jan 31, 2016

@bcoe Works for me 👍

@jameswomack
Copy link

👍

@isaacs
Copy link

isaacs commented Jan 31, 2016

Can't speak to pm2, iisnode, etc.

But if they're using the stock src/node.js and lib/module.js (or even abusing it, as spawn-wrap does), then that'll be the fully resolved and realpath'd filename of the first filename passed to node (ie, the main bin), on all supported platforms including Windows.

@bcoe
Copy link
Member Author

bcoe commented Feb 1, 2016

Boom, I've written:

https://github.com/yargs/require-main-filename

And stood up a Node Azure instance that I've tested on using:

https://github.com/bcoe/test-node-path (which we can use for debugging future edge-cases).

I'm pretty confident with this approach at this point. We might occasionally run into a process runner that steps on our toes but we can simply update require-main-filename when this happens.

@sindresorhus
Copy link
Member

I guess this works fine for the use-case of yargs, but it gets pretty weird if someone wants to use yargs-parser in a reusable module that is consumed by something else. How it works should be clearly documented.

@bcoe
Copy link
Member Author

bcoe commented Feb 1, 2016

@sindresorhus 👍 I will expand on this:

These features can be turned on and off by using the yargs configuration stanza in your package.json.

Further before merging, explaining that it looks for a yargs key in your application's top-level package.json.

@sindresorhus
Copy link
Member

These features can be turned on and off by using the yargs configuration stanza in your package.json.

And be clear that it should be turned off if you plan on using this in a reusable module and not in a CLI or app.

@nexdrew
Copy link
Member

nexdrew commented Feb 1, 2016

I still think we should consider doing the package.json lookup in yargs and explicitly pass the config options to yargs-parser via public API.

That may resolve some of the concerns brought up here and would reduce the coupling b/w the 2 modules, making yargs-parser more reusable. Thoughts?

@bcoe
Copy link
Member Author

bcoe commented Feb 1, 2016

@nexdrew there are two reasons I'm not too excited about this:

  1. I'd like you to be able to use the yargs-parser module in isolation (if you want a good argument parser without the yargs UI). and it's my expectation that yargs-parser would be configured in an identical way when used in isolation, vs. when used as a dependency of yargs -- you simply set a yargs stanza in your package.json.
  2. my goal of splitting the parser out from the core yargs library is to reduce the logic in yargs itself.

I honestly don't fully understand the use-case of wanting to be able to use yargs-parser in isolation, but not wanting the ability to configure it?

@bcoe
Copy link
Member Author

bcoe commented Feb 1, 2016

@nexdrew #4

^ I think I see where you're coming from, and this might get us the best of both worlds?

@isaacs
Copy link

isaacs commented Feb 1, 2016

I'm kind of with @nexdrew and @sindresorhus on this.

The use case isn't using yargs-parser without configuring it. It's about using yargs-parser and configuring it in a different way.

yargs-parser should be just a parser. If you want to remove the "get the yargs config stuff out of package.json" to also be abstracted out, that's great. Yargs can do yargsParser(yargsConfigurator()) easily enough. Otherwise it retains the same tight coupling it already has as an internal component.

But if I wanted to use the yargs-parser in, say, nopt or dashdash, it's pretty weird to have to override it or else have it pull a yargs field out of package.json. With that implicit behavior in the parser, it's not actually abstracting out the parser itself, it's just moving the same tightly coupled code to another place. (See the many unusable modules of node-tap in its early 0.x days, or some of the failed-to-pull-out bits of npm; it's an easy mistake to make and I've made it myself a lot of times.)

@bcoe
Copy link
Member Author

bcoe commented Feb 1, 2016

I can't argue with three people's instinct. As much as I'd like to stop bike-shedding this thread, I will continue bike-shedding this API.

@jameswomack
Copy link

I agree with the esteemed collaborators above, with a slightly different take on it. I could see a module with both CLI parsing and pkg configuration, but that becomes a highly specialized configuration module rather than a CLI parser that follows the UNIX philosophy of doing one thing and one thing well.

bcoe added a commit that referenced this pull request Feb 6, 2016
based on various discussions I've switched to require.main.filename
@bcoe bcoe merged commit d589474 into master Feb 6, 2016
@bcoe bcoe deleted the require.main.filename branch February 6, 2016 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants