kevinbeaty / any-promise

NOTE: You probably want native promises now
MIT License
179 stars 17 forks source link

Look for custom keys in top-level package.json #2

Closed addaleax closed 8 years ago

addaleax commented 8 years ago

This implements one of the approaches discussed in #1, namely looking for the top-level package.json file by walking up the dependency tree and looking for:

and using the results to determine which Promise library to load.

This is not necessarily a suggestion for the final implementation, but rather a starting point for discussion, I guess. :smile:

kevinbeaty commented 8 years ago

Thanks for starting the discussion :)

I'm actually very uneasy with this approach after seeing it. I'll have to give it some thought.

Do you know of any other packages that do a similar thing?

kevinbeaty commented 8 years ago

Could we make use of require.main.filename to find the top level module?

julien-f commented 8 years ago

And then use pkg-conf from this file?

kevinbeaty commented 8 years ago

pkg-conf looks useful.

A useful discussion re. different approaches: Determine project root from a running node.js application

addaleax commented 8 years ago

@julien-f pkg-conf looks for the closest package.json, not the top-level one, which makes it rather unusable for the npm3 situation, if I don’t miss anything? It looks like interesting though, yes, and probably having a part of this functionality in another package is a good idea.

@kevinbeaty And I’ve thought about require.main.filename, but that won’t work for scripts provided by dependencies (e.g. a test runner) or in subdirectories like bin/, so I’d actually feel a little more uncomfortable with using that one…

julien-f commented 8 years ago

@addaleax It works if the search starts from require.main.filename.

kevinbeaty commented 8 years ago

@addaleax True, but for a test runner we could use the current approach using an PROMISE_IMPL env variable.

Using app-root-path combined with pkg-config could work well (this was mentioned in the SO answer above)

addaleax commented 8 years ago

Mh, it’s not only test runners which would be affected by using pkg-config, but almost anything in node_modules/.bin in general.

But using app-root-path sounds good and I’ll update my PR with it. :)

kevinbeaty commented 8 years ago

Actually I think the closest package.json to app-root-path is exactly what we want, so pkg-conf would work well if we combine it, correct? Or am I missing something?

The issue with the test runners is more an issue of getting the correct "app-root" path, not necessarily the correct package.json. It won't solve all corner cases, but it will solve many.

addaleax commented 8 years ago

Yes, app-root-path + something like pkg-conf sounds like the best solution to me, too – sorry if that wasn’t clear. :)

addaleax commented 8 years ago

What do you think of the idea of peeking into the dependencies + devDependencies?

pkg-conf is not really interested in returning more than one entry from the found package.json (i.e. only any-promise), but if you don’t think we should inspect the deps anyway, that should not be a problem. I kinda like this idea, though, especially since it still makes this module feel more or less automagic.

kevinbeaty commented 8 years ago

For now, I think it would be better to be explicit and only look for a custom key.

I'm currently thinking something like this:

  1. Use ANY_PROMISE env if set
  2. Use PROMISE_IMPL env if set (backwards compatible)
  3. (Maybe) Use global.ANY_PROMISE if set (user can set at app load)
  4. (Maybe) Use custom "any-promise" value in app root path package.json if found and set (this PR)
  5. Use global.Promise if set
  6. Throw error

And remove the priority list lookup. I'm up for alternative names as well. Not just "ANY_PROMISE"

But you might be able to change my mind.

addaleax commented 8 years ago

Okay, updated this PR with your suggestion – only look for something like "any-promise": {"preference": "bluebird"} in the top-level package.json, using app-root-path and pkg-conf.

kevinbeaty commented 8 years ago

Thanks. I'm going to let this one sit for a bit to allow anyone (maybe even me) to raise objections. If we do merge it, we should come up with some tests. But no need to do that until we decide we really want to do it.

Thanks for your work.

kevinbeaty commented 8 years ago

Closing in preference of #4