support parent param to facilitate sourcemap usage

[iambumblehead]

@jakebailey the changes here allow this call to be used from the test case (which I'll try copying into this PR --thank you)

    const indexModule = await esmock("../index.js", {
        os: {
            hostname: () => expectedHostname,
        },
    }, null, {
        parent: import.meta.url
    });

re #113

[iambumblehead]

@jakebailey your work is basically copied here. Thank you for the excellent test and solution. Please give your approval (or disapproval) and if things are okay here the changes can be published

[iambumblehead]

tests are passing :)

[jakebailey]

I guess my worry is that anyone using TS is going to have this problem, and not know that they have to pass in their URL via some opts parameter to get this to work correctly. The signature of the top-level function isn't really conducive to making that obvious in the d.ts file, either, in that you can't make opt.parent required because it's the last parameter after a bunch of optional ones.

If it were me, the API would be rewritten like:

declare function esmock(path: string, parent: string, localmock?: Record<string, any>, globalmock?: Record<string, any>, opts?: any): any;
export default esmock;

That is, force the caller to provide their URL. But, this would be a breaking change / major version bump.

If backwards compatibility is a concern, it could instead be defined as two overloads:

declare function esmock(path: string, parent: string, localmock?: Record<string, any>, globalmock?: Record<string, any>, opts?: any): any;
declare function esmock(path: string, localmock?: Record<string, any>, globalmock?: Record<string, any>, opts?: any): any;
export default esmock;

Then, you can determine which version of the function is being called by checking to see if the second parameter is a string or not. If it isn't, then you can shift the arguments over and continue.

However, I'm not sure that it's a good idea to infer the caller's filename, since it doesn't seem to be accurate and it's hard to to tell what went wrong besides a .ts file in the error message. It seems that based on import-meta-resolve's choices that inferring the caller isn't generally doable when source mapping is involved, hence requiring a parameter.

[iambumblehead]

Thanks for this thoughtful reply. Essentially I agree.

It seems operator overloading would be the most user friendly way of supporting both interfaces, the current interface and a new interface that specifies the parent. If you agree with the operator overloading approach, let's do that.

If you would submit the types file that best describes that interface, I'll update esmock to support whatever you define there. If we should use the variants you've given above, I'll use those.

BUT, what do you think of this idea... what if we support something like this, where parent is defined near the top of the test file one time?

import test from "ava";
import esmock from "esmock";
import { fileURLToPath } from "url";

import type * as indexType from "../index.js";

const expectedHostname = "my-machine";
const esmockdist = esmock.parent(import.meta.url); // <-- define parent one time, at the top of the file

test("using relative path", async (t) => {
    const indexModule = await esmockdist("../index.js", {
        os: {
            hostname: () => expectedHostname,
        },
    });

    const getHostname: typeof indexType.getHostname = indexModule.getHostname;
    t.is(getHostname(), expectedHostname);
});

[iambumblehead]

for the last suggestion it is probably possible to reuse the same esmock instance without having to define a new one, eg

-const esmockdist = esmock.parent(import.meta.url);
+esmock.parent(import.meta.url);

in this case, esmock creates a mapping between "parent" values and dynamically detected locations, reusing the parent url each time its associated location is detected

I personally prefer the more explicit variant const esmockdist = esmock.parent(import.meta.url) but the shorter variant is OK.

[jakebailey]

I wouldn't want to have any sort of global side effect, but feasibly the one where you assign it to a local and then use it is possible, though maybe a little awkward. I sort of feel like this is worth the API break to get right.

I'm a little busy today, but how would you feel if I (later today, or tomorrow) write out a type definition that's a little more complete to show what I would want, and go from there?

[jakebailey]

Either way, thanks for working on this project; I'm using it on a yet-to-be-released package for testing and it was the only module mocking lib that seemed to work (besides this one bug, which I've worked around).

[iambumblehead]

@jakebailey that sounds good.

I want to say, for myself, I do not use a transpiler and do not wish to define "parent" for every esmock call, so if we could continue supporting the current smaller interface that doesn't require "parent" that would be great for me.

cc @aladdin-add @tripodsan feel free to ignore me and if you have any opinions or advice feel free join the discussion.

[jakebailey]

Ah, well, if that's hard restriction, then I guess an overloaded signature is going to be the only way, unless there's a node/v8 API that gives a better answer for the caller path than new Error.

I'll also note that this means that one can't write a helper function declared in another file that calls esmock, I think, because the call frame above esmock would be the helper, and not the actual expected calling file. (Even finding a better call stack API would not fix this case; only explicitness helps.)

[jakebailey]

When I get some time, I'll branch off of this PR and test a few things.

[iambumblehead]

Thank you. You have some insights I don't have and your advice is appreciated by me.

[iambumblehead]

the tests have a false positive in them atm -- the new tests fail in the windows environment

   ✖ using absolute path Rejected promise returned by test
  ✔ using relative path

I don't remember the details around why the tests are being called this way but some unit-tests would pass but returned failing exit codes, so the "true" part was appended to the end. It needs to be updated to pass and fail when the tests pass and fail

   "test:test-source-map": "cd tests-source-map && npm test || true",

[iambumblehead]

the tests are updated to correctly pass and fail #117

[iambumblehead]

the error is here https://github.com/iambumblehead/esmock/blob/master/src/esmockModule.js#L166-L174 and likely originates at resolvewithplus, where a ticket is added.

We could ignore the error on windows now and follow up with a solution later.

[iambumblehead]

the win32 error can probably be resolved by the end of the day today

[iambumblehead]

@jakebailey the windows errors should be resolved. If you have time, would you verify if things are resolved for your package and unit-tests? Defining esmock dependency this way should bring this branch,

    "esmock": "iambumblehead/esmock#113-support-parent-param"

Assuming things are working okay, I'm interested to try implementing an interface you would add for defining the parent.

[jakebailey]

Yes, I'll take a look. I didn't get a chance earlier today, but hopefully tomorrow or so. Sorry to keep you waiting.

I'm not sure which Windows issue you mean, unless you mean the test skips I applied in hereby, though.

[jakebailey]

FWIW the Windows tests still fail in my project, but there are fewer errors (if you replace test.skip with just test); I only have read-only access from my Windows machine at the moment so I can't give you a branch, but I can fix that tomorrow when I have more time to fix my broken git setup (I do most things on other machines).

I've been looking into the stack trace option, and I haven't found something that wouldn't just be a wrapper of Error.stack parsing, and I think it'd be undesirable anyway given the inability to make helpers.

I was hoping that it'd be possible to have the error message thrown mention setting the parent if .ts files are unexpected, but I know that you also display support for ts-jest too, where you actually do want to resolve to .ts files in the src directory, so I'm not really sure what to do there. If the import.meta.path parameter were required, I would think it'd "just" work out, as IIRC ts-jest would have that path set to the .ts file, but I might be misremembering and you seem to want the implicit behavior anyhow.

EDIT: But, my tests might be bugged because I'm not sure that esmock (or other similar projects) support mocking in parallel; doing that properly probably involves reimplementing state via async_hooks's AsyncLocalStorage.

[iambumblehead]

It seems there are multiple issues that could be resolved.

I setup a virtual-box windows system to take a closer look at the interesting windows-related issues you're encountering. It will be great to understand those and reproduce them with unit-tests.

esmock should resolve .ts and .tsx files. I hope everything can be resolved soon and am basically available on Saturday to resolve and improve things as much as possible.

I appreciate your help and showing these issues to me and don't want to abuse your time, so feel free to ignore this request, but if you could reproduce the windows error inside this branch's unit-tests that would be super. From esmock's root folder use npm install && npm run test:install to fetch all dependencies needed, then from whichever test folder is being used, npm test will run tests.

[jakebailey]

I appreciate your help and showing these issues to me and don't want to abuse your time, so feel free to ignore this request, but if you could reproduce the windows error inside this branch's unit-tests that would be super. From esmock's root folder use npm install && npm run test:install to fetch all dependencies needed, then from whichever test folder is being used, npm test will run tests.

No problem, I'll give it a shot, but I might not get to that until this weekend; I'm busy with a number of things, last weekend was just a lucky period where I had a lot of free time to try things out.

[iambumblehead]

yes esmock supports parallel tests

[jakebailey]

Oops, I neglected to actually completely remove my hack and replace it with the parent opt; when I actually do it right, all of my tests pass! 😅

I guess that means that the behavior of this PR is solid; I will try and give you an example of a potential API (and implementation in pure JS) when I'm able.

[iambumblehead]

that is terrific news thanks for sharing that :)