Fix dir of getNameAndVersion for require package.json

[jhen0409]

Related to #439.

Also, we don't need to check !pkg, because it will throw module not found error if no package.json file found.

[malept]

Thanks for the fix! Could you add a test for this so we don't have a regression in the future?

[MystK]

This is also still broken on Windows

EDIT: nevermind my bad! Looks like my global electron-packager was being used instead of my local. Works great. :)

[malept]

@jhen0409 I assume the test fails without your fix?

[malept]

@jhen0409 Hmmm, the test seems to fail with the fix too. (You should run npm test before you commit.)

[jhen0409]

@malept Sorry 😂, it should fixed.

[malept]

If someone could make sure that the test fails without the fix applied, I would appreciate it. Otherwise I'll test it later today (or tomorrow depending on how much time I have available).

[jhen0409]

I assume the test fails without your fix?

This is test fail log without my first commit:

  dir argument test: should work with relative path

module.js:442
    throw err;
    ^

Error: Cannot find module '../fixtures/el-0374/package.json'
    at Function.Module._resolveFilename (module.js:440:15)
    at Function.Module._load (module.js:388:25)
    at Module.require (module.js:468:17)
    at require (internal/module.js:20:19)
    at getNameAndVersion (/Users/Jhen/github/electron-packager/index.js:5:1220)
    at packager (/Users/Jhen/github/electron-packager/index.js:11:3734)
    at Test.<anonymous> (/Users/Jhen/github/electron-packager/test/basic.js:479:3)
    at Test.bound [as _cb] (/Users/Jhen/github/electron-packager/node_modules/tape/lib/test.js:63:32)
    at Test.run (/Users/Jhen/github/electron-packager/node_modules/tape/lib/test.js:82:10)
    at Test.bound [as run] (/Users/Jhen/github/electron-packager/node_modules/tape/lib/test.js:63:32)

[malept]

@jhen0409 thanks!

[SimulatedGREG]

@malept
(Updated to 7.5.1 and can confirm with npm ls electron-packager.)

Sorry to report again, but 7.5.1 did not solve my issue. It would seem that...

getNameAndVersion(opts, path.resolve(process.cwd(), opts.dir) || process.cwd(), function (err) {
  ...
})

...assumes that the package.json that contains either electron or electron-prebuilt exists inside the given dir path or inside the process.cwd() when a dir path isn't give, but fails to also check inside process.cwd()with a given dir. This is some-what common when using a 2 package.json electron setup. I believe reversing the OR statement from

path.resolve(process.cwd(), opts.dir) || process.cwd()

to

process.cwd() || path.resolve(process.cwd(), opts.dir)

should do the trick and works for my setup as well.

[malept]

@SimulatedGREG Could you please file a PR (with a testcase)? I will review as soon as I can.

[malept]

@SimulatedGREG Hmmm. Now that I think about it, doesn't that change give the current working directory priority over the specified opts.dir? That seems counterintuitive.

[SimulatedGREG]

@malept
It may have that effect, but like I sorta mention before, this wasn't a problem with get-package-info. 😕

I can make a PR later today, but not comfortable with doing tests so I'll try my best.

[malept]

I am also not opposed to putting get-package-info back, assuming someone fixes the issue that @zeke mentioned here: #435 (comment)

I can make a PR later today, but not comfortable with doing tests so I'll try my best.

@SimulatedGREG The important part about the test is creating a minimal fixture that exemplifies the problem that you're seeing. I can help with the code part.

[SimulatedGREG]

@malept

@SimulatedGREG Hmmm. Now that I think about it, doesn't that change give the current working directory priority over the specified opts.dir? That seems counterintuitive.

From what I can see, I don't believe reversing the OR statement would be problem. Even if the current working directory had priority over opts.dir there shouldn't have a 2 package.json setup the requires electron/electron-prebuilt in both files. (But I do see your point).

I made an adjustment in my fork which simply checks the parent directory if a electron/electron-prebuilt version isn't inferred from opts.dir's package.json, but its a little repetitive. https://github.com/SimulatedGREG/electron-packager/commit/ac53980e8e0139d5379b832d2c4e40dc70f12bcb

[malept]

Reversing the OR statement would be a problem because process.cwd() is never null/false, so it would never short-circuit.

I would prefer re-adding get-package-info over hardcoding looking at a parent directory's possible package.json.

[SimulatedGREG]

Now I see the problem better. I'll try getting get-package-info working if possible.

[zeke]

Reached out to @substack to see about getting that upstream-upstream resolve thing fixed. If not, we could update get-package-info to depend on a fork of resolve...

[malept]

The other alternative is to fix rahatarmanahmed/get-package-info#2.

[rahatarmanahmed]

I'd be happy to accept a PR for rahatarmanahmed/get-package-info#2, I think that's the better, more immediately fixable solution than getting substack to merge the resolve PR.

[zeke]

short term fix: #445