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

Feature Request: Extend package.json configuration search beyond first file found #439

Closed
mhassan1 opened this issue Feb 19, 2021 · 10 comments
Labels
enhancement released on @beta released Pull Request released | Issue is fixed

Comments

@mhassan1
Copy link
Contributor

Versions

  • NodeJS: 14.15.1
  • mongodb-memory-server-*: 6.9.3
  • mongodb: 4.4.1
  • mongoose: 5.11.15
  • system: MacOS

package: mongodb-memory-server-core

What is your question?

In resolve-config.ts, we look for the closest package.json and stop when we find it (even if it doesn't contain any configuration information):

/**
* Find the nearest package.json for the provided directory
* @param directory Set an custom directory to search the config in (default: process.cwd())
*/
export function findPackageJson(directory?: string): void {

In the case of a monorepo, the configuration for the postinstall script must be in the root of the monorepo (since INIT_CWD is the root), but the configuration for the package in the packages directory of the monorepo must be in the package.json in that package (since the process.cwd() is the package). We need to include the same configuration in both places.

It would be useful to be able to include the configuration only in the root so it takes effect in both situations. A couple of ways we could do it:

  • Extend findPackageJson to merge all package.json files that it finds as it traverses up the hierarchy. It could be something like:
export function findPackageJson(directory?: string): void {
  const _packageJsonConfig = {};
  const finderIterator = finder(directory || process.cwd());
  let foundPackageJson;
  while ((foundPackageJson = finderIterator.next())) {
    if (foundPackageJson.done) {
      break;
    }

    const { value, filename } = foundPackageJson;

    log(`Found package.json at "${filename}"`);
    _.defaultsDeep(_packageJsonConfig, value?.config?.mongodbMemoryServer || {});
  }
  packageJsonConfig = _packageJsonConfig;
}
  • Alternatively, we could stop when we see the first package.json that contains config.mongodbMemoryServer
export function findPackageJson(directory?: string): void {
  const _packageJsonConfig = {};
  const finderIterator = finder(directory || process.cwd());
  let foundPackageJson;
  while ((foundPackageJson = finderIterator.next())) {
    if (foundPackageJson.done) {
      break;
    }

    const { value, filename } = foundPackageJson;

    if (!value?.config?.mongodbMemoryServer) continue;

    log(`Found package.json at "${filename}"`);
    _packageJsonConfig = value.config.mongodbMemoryServer;
    break;
  }
  packageJsonConfig = _packageJsonConfig;
}
@hasezoey
Copy link
Collaborator

hasezoey commented Feb 20, 2021

@nodkz is there any reason on why we should not do this?
if not, i would include this in 7.0

but i guess, #440 would need to be fixed first

@hasezoey
Copy link
Collaborator

@mhassan1 it would need to be debated if the first found should take higher priority, or the last found
(like /root/project's package.json overwrites values in /root or the other way)

i guess currently it is this way because of these problems (which hierarchy & conflicting configs)

@nodkz
Copy link
Owner

nodkz commented Feb 20, 2021

Definitely, this will be very useful for workspaces & monorepos.
@mhassan1 it's a great idea. Would you like to do a Pull Request on beta branch?

is there any reason why we should not do this?

Only a lack of logging information can stop us. It's too bad when the library works somehow weird and you cannot understand why.

merge all package.json OR stop on the first config.mongodbMemoryServer

It's a very interesting question. I think that we need to use the first found config. I think that merging can contain bugs and hard for resolving on the end-users side. Anyway, we may start from the first found and in the future improve it by adding merge logic. BUT for this, we need to gather more real use cases when merging really worth it.

@mhassan1 if you have a good example of why needs merging (which properties should be on what levels in your repo) - please share with us. Tnx.

@mhassan1
Copy link
Contributor Author

I actually don't have a use case for merging multiple files, and I agree that it could be harder for the user to understand. I will offer a PR on beta for stopping at the first file that contains configuration.

@mhassan1 mhassan1 changed the title Feature Request: Merge config from all package.json files in the hierarchy instead of stopping at the first one found Feature Request: Extend package.json configuration search beyond first file found Feb 20, 2021
@hasezoey
Copy link
Collaborator

@nodkz @mhassan1 i created an PR for it, if you dont mind #441

@nodkz
Copy link
Owner

nodkz commented Feb 21, 2021

@hasezoey for me it looks very good 👍

I think we can publish a new beta version. And await feedback from @mhassan1.
Thanks a lot!

@hasezoey
Copy link
Collaborator

@nodkz i just want to wait for some feedback from @mhassan1 if this pr is fully doing what this issue evolved into

@mhassan1
Copy link
Contributor Author

LGTM. Thanks for the quick turnaround.

@github-actions
Copy link

🎉 This issue has been resolved in version 7.0.0-beta.18 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Jul 1, 2021

@github-actions github-actions bot added the released Pull Request released | Issue is fixed label Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement released on @beta released Pull Request released | Issue is fixed
Projects
None yet
Development

No branches or pull requests

3 participants