Pass along options argument

[mikew]

Closes https://github.com/ilearnio/module-alias/issues/53

[mikew]

Not sure why the test is failing for latest node. The others make sense, as it seems options.paths for require.resolve only came in nod@8.

Tests all pass here (node@10), and with this change, the madness that is TypeScript Paths + Jest + compiled + non-compiled all work, after days of frustration.

[Kehrlann]

Thanks for contributing ! I've been a bit busy, but I'll try to take a look pretty soon.

[mfaheemakhtar]

@mikew, the latest version of NodeJS (12) has some changes in module.

I think this could be an issue https://github.com/nodejs/node/pull/23683/files

https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V12.md

[Kehrlann]

Good catch @mfaheemakhtar !

It means that calling resolve will start to look for a node_modules. Update the folder structure like so:

And change the test to something like:

  it('should not break require.resolve', function () {
    require.resolve('require-resolve-module', {
      paths: [path.join(process.cwd(), 'test', 'src', 'resolve')]
    })
  })

It should work.

---

For older versions of node, I think you might have to fiddle with process.version to include / exclude the test you added.

[Kehrlann]

@mikew dev has been updated ; the "reset()" spec shouldn't fail on node 11 and 12 anymore.

[Kehrlann]

Hey @mikew ! This PR hasn't seen work in a while. Do you wish to wrap it up, or should I pick up where you left off ?

[mikew]

Sorry for the delay, I've been away from work.

The issue with the test was fixed in Node v12.3.0. The test could also just be changed to use a spy and ensure the arguments that shouldn't be mucked with aren't mucked with.

[mikew]

All green. Feel free to bring in sinon for proper spies.

[ilearnio]

Hi @mikew. Added a couple suggestions, please have a look when you are free. Thank you for dedicating your time! You are really helping a lot!

[Kehrlann]

Nice PR, I like the idea of pulling in semver ! Thanks again for your time and commitment.

Personnally I wouldn't spy on the call. I'd drop spying, and drop the try-catch in your test - instead, use the folder structure I suggested in my previous comment. The goal is to use the public API and the "normal" behavior of require to validate that your change does The Right Thing™.

If you don't pass along options args, the test will blow up. If you do pass them, test will succeed, which demonstrates that your implementation works.

@ilearnio, do you agree with this strategy ?

[ilearnio]

@Kehrlann Good point. Yeah, it seems like spying is not necessary, we can just check if the require.resolve does not throw error. Also would be good to test the returned value of the require.resolve

[mikew]

The intention was to not rely on how require.resolve with the options argument works in tests, as that functionality had a regression in 12.0.0 - 12.2.0.

Changing the folder structure isn't needed, and frankly needing node_modules to exist doesn't make much sense.

Anyways, just wanted to pass an argument. Feel free to continue the work.

[Kehrlann]

Phew sorry, it's been more than a month, I've gone through a space-time breach.

I've had time to look at this PR again ; it looks good to me, specifically because of the regression in node 12.0 to 12.2 included. I'm also okay not bringing in Sinon.

I've added a commit to this PR, and tested using the require.resolve behavior. The result is in #64.

@ilearnio if you have time, could you please review both and decide what you like best ? You can close the other one.

[ilearnio]

@Kehrlann approved your PR, looks great. Does the same as this PR but without spy module 👍

[ilearnio]

@mikew this it's now released in v2.2.1. Thanks!