[poll] is it a good idea?: rename 'partial mock' to 'merged' or 'blended' mock

[iambumblehead]

Is it a good idea to rename 'partial mock' to 'merged' or 'blended' mock?

If you're cc'd here feel free to ignore or give an opinion whichever is fine, cc: @Swivelgames @tripodsan @aladdin-add @jakebailey

Currently, "partial mock" is described this way in the README (and the wiki link is here),

// use esmock.px to create a "partial mock"
const pathWrapPartial = await esmock.px('../src/pathWrap.js', {
  path: { dirname: () => '/home/' }
})

The words "merged", "blended" and "assigned" are more accurate imo. What do you think?

// use esmock.m to create a "merged mock" from  the original module definition
const pathWrapPartial = await esmock.m('../src/pathWrap.js', {
  path: { dirname: () => '/home/' }
})

The interface could be changed when the next major release happens and, if no one comments, the interface will probably be changed.

cc @vueme who is also using the "px" interface. Please feel free to give any opinion you might have.

[jakebailey]

I think "partial" is the correct term to use, but I would recommend renaming those functions to be more descriptive, e.g. to explicitly call this function partial.

But, I might be confused as to the difference between p, px, etc; it wasn't clear to me what they did, but at least in my case, I never wanted a partial mock anyway.

[tripodsan]

the recent change to make the non-partial mock the default was a bit of a surprise (and broke many of our tests :-). I think that most mocking needs are partial...

In any case, partial is good term.

before ESM, I used proxyquire, and they call it callThru.

alternatively, you could change the way existing methods are proxied, eg with:

// use '*' to create a "partial mock"
const pathWrapPartial = await esmock('../src/pathWrap.js', {
  path: { dirname: () => '/home/' },
  '*': true,
})

or similar to proxyquire:

// use '*' to create a "partial mock"
const pathWrapPartial = await esmock('../src/pathWrap.js', {
  path: { dirname: () => '/home/' },
  '@callThru': true
})

[iambumblehead]

I think that most mocking needs are partial...

I agree with this and would actually prefer if partial mocking were the default behaviour (again). Partial mocks work for all tests "partial" or "not partial". Instead of an option to enable partial mocking, there could be an option to disable partial mocking, eg like this,

const pathWrapPartial = await esmock.strict('../src/pathWrap.js', {
  path: { dirname: () => '/home/' }
})

[jakebailey]

Regarding:

// use '*' to create a "partial mock"
const pathWrapPartial = await esmock('../src/pathWrap.js', {
  path: { dirname: () => '/home/' },
  '*': true,
})

I find this confusing, because the object passed is a mapping of import specifiers to contents, not an options bag. I wouldn't recommend using the same parameter for two things.

As for partial default or not, I definitely prefer not having it partial by default; this matches jest's mocking behavior: https://jestjs.io/docs/next/mock-functions#mocking-partials

I worry about things like unwanted side effects that come fell mocking a library that does things like run processes or make requests. E.g., if I mock child_process, and change my program to use exec instead of spawn, it'd be a bad time if my tests started actually calling things just because I hadn't yet mocked it.

[iambumblehead]

We could rename 'px' to 'partial' and if anyone feels strongly about it, it could be renamed.

There is a quote I read from Bjarne Stroustrup that I could not find, but basically what he says is, longer names can be better for things that are used less frequently, shorter names can be better for things that are used more frequently.

I was previously using rewiremock for cjs tests and my rewiremock calls looked something like this and the equivalent esmock call shorter (which I prefer)

const file1 = await rewiremock.proxy(async () => await import('./path/to/file.js'), {
  'dependency':  stub
});
const file2 = await esmock('./path/to/file.js', {
  'dependency': stub
});

"px" could be better in situations where esmock is used more frequently

const file1 = await esmock.partial('./path/to/file.js', {
  'dependency': stub
});
const file2 = await esmock.px('./path/to/file.js', {
  'dependency': stub
});

I like the idea of removing "px" and supporting a "strict" function instead, but no one else here commented in support of that idea so... IMO the idea that "px" is dangerous comes from an OOP mindset that should be considered an edge case rather than the default case, and therefore "px" should be the default behaviour, imo. It is nice to follow the jest behaviour, but esmock is not jest. esmock is made by a person and its purpose is to help other people.

There are not many users but if those users do not want "strict" to be the default behaviour and want "px" or "partial" to be separated then I support that. If there are no comments after a few days, let's close this issue

[iambumblehead]

SyntaxError {
  message: 'The requested module \'./the-module.mjs\' does not provide an export named \'the-named-export\'',
}

If partial-mocking is the default behaviour, new users won't encounter this error, which happens when a mocked module does not define an imported name. For this reason, it might be better to use partial mocking by default and strict mode as optional

[jakebailey]

If I'm mocking a library, 90% of the time it's because it has side effects. I think I'd be pretty frustrated if I was doing a refactor and my tests started executing commands because I was replacing exec with spawn, or some API call with another, and I hadn't yet updated my tests to mock that call. (But, I'm repeating myself.)

I'm not aware of any module mocking library that defaults to a partial mock. I'm not sure it's a good idea for it to be the default; if mocking doesn't work for some reason without partial, then that feels like a bug.

FWIW "partial" is already a very short name; I'm sure it's a lot shorter than the long names that might appear in C++. A two letter shortening is fine for a local variable, but for a public API, I think it's not very descriptive. I would say that pretty much all of the time, devs don't like to read docs, so any hint you can give in an API is good.

[iambumblehead]

Thanks for the convincing reply. Maybe, to avoid that error, names that aren't defined by the mock could be exported as "esmockstub" or something similar.

[jakebailey]

I'm not sure what you mean; doesn't that error imply that the test code just needs to add what the importing code is using? That seems like a good error to me, as it informs the user that their mock is not complete yet.

Potentially, this could have a better error (somehow), if readability is a concern.

[tripodsan]

I'm not aware of any module mocking library that defaults to a partial mock.

yes, proxyquire.

and I think, many users that switch to ESM, replace proxyquire with esmock (as there is no good alternative).
so maybe, call it proxymport instead of esmock, and we're good :-)

[jakebailey]

I'm coming from a background of jest mocks (and other mocking libraries for objects like sinon, ts-mockito, typemoq, ts.moq), all of which aren't partial like that; I guess I would have expected a similar view (especially as jest doesn't really have ESM mocking yet; part of the reason I'm here)

[iambumblehead]

I'm not sure what to think so am not replying. Both "strict" and "partial" are supported and the default behaviour should probably be one that is preferred by most users. But there aren't many users.

@jakebailey's replies here are convincing and subjectively I also relate to the proxyquire experience mentioned by @tripodsan

[iambumblehead]

@Swivelgames @aladdin-add @cawa-93 @gmahomarf politely, would you share your opinion on this question: which should be the default esmock behaviour?: "partial" mocking or "strict" mocking?

To demonstrate the difference, a target module might look like this,

import { basename, dirname } from 'path'

const apath = '/path/to.png';
console.log(dirname(apath), basename(apath))

Here's what happens when partial or strict mocking are used. Note the "partial" and "strict" functions don't exist on esmock now, they are used for demonstration. Currently "strict" is default and there is a partial function named "px"

import esmock from 'esmock'

// mock definition is merged with path definition, including 'dirname'
esmock.partial('./logpath.js', {
  path: {
    basename: () => 'mockedbasename'
  }
}) // "mockedbasename to.png"

// mock definition is not merged with path defintion. "dirname" must be explicitly defined here,
esmock.strict('./logpath.js', {
  path: {
    basename: () => 'mockedbasename'
  }
}) // Error "The requested module 'path' does not provide an export named 'dirname'"

[cawa-93]

@Swivelgames @aladdin-add @cawa-93 politely, would you share your opinion on this question: which should be the default esmock behaviour?: "partial" mocking or "strict" mocking?

To demonstrate the difference, a target module might look like this,

import { basename, dirname } from 'path'

const apath = '/path/to.png';
console.log(dirname(apath), basename(apath))

Here's what happens when partial or strict mocking are used. Note the "partial" and "strict" functions don't exist on esmock now, they are used for demonstration. Currently "strict" is default and a the partial function is named "px"

import esmock from 'esmock'

// mock definition is merged with path definition, including 'dirname'
esmock.partial('./logpath.js', {
  basename: () => 'mockedbasename'
}) // "/path/ to.png"

// mock definition is not merged with path defintion. "dirname" must be explicitly defined here,
esmock.strict('./logpath.js', {
  basename: () => 'mockedbasename'
}) // Error "The requested module 'path' does not provide an export named 'dirname'"

After a quick look at the documentation, I get the impression that you only mock what you specified. In other words, call esmock('module.js') should work identically to the native import('module.js') and not throw an exception. This, as I understand it, corresponds to the behavior of partial mokck. In addition, it will allow to make changes in the modules without making changes to the tests

[iambumblehead]

let's wait a day or two and see if anyone else comments

[gmahomarf]

Hey, I appreciate you asking for my input.

So far, I agree with @jakebailey. Borrowing the existing terminology in this thread, "strict" is what I have come to expect the default to be, regardless of the mocking library. I could go on for hours about the reasons why I believe this, but I'll summarize instead.

The component under test should be fully under one's control. From imported constants, to functions and entire classes, unit tests are supposed to validate behavior under specific conditions. Side effects are definitely unwanted and disruptive in this scenario, and "strict" mocking ensures that nothing slips through the cracks. Unit tests should also not fail if one of the module's dependencies changes the way it works, which is a probability with partial mocks. That's what integration tests are for. For those who practice TDD, it should in theory never be an issue, because you shouldn't have production code without a proper test covering said code first, including mocking dependencies. For those of us who sometimes stray from the TDD formula, it's comforting to know that any new dependencies that we add to a module will outright break the unit test, and will be a good reminder to update it, no matter how rushed we're feeling that day.

I also agree that most people will be coming to esmock from proxyquire (I am one of those people). However, I actually defaulted to using proxyquire's noCallThru() to enforce "strict" mocks. As mentioned above, when unit testing, it's best to have full control over the component being tested.

On a personal note, I choose to not use "partial" mocking, unless absolutely necessary. That being said, if partial mocking is implemented, I wholeheartedly agree with having the method called something more descriptive than px. There's no reason both functions can't exist as aliases of each other, and I was actually planning on opening a feature request for such a case, based on the single instance of partial mocking I've done so far 😄

At the end of the day, though, I strongly believe in the freedom granted by open source, which means that, if a choice is made that I don't agree with, I can always fork or find another project, as can everybody else. I do hope that, if the final decision results in making partial mocking the default, proper semantic versioning is observed, and the required release notes and changelogs include mentions of that breaking change and, if possible, a migration guide.

[iambumblehead]

Based on the discussion, my sense is to force the user to import one of these,

import esmock, { esmock, esmockStrict } from 'esmock/partial'
import esmock, { esmock, esmockPartial } from 'esmock/strict'

and remove support for this altogether,

import esmock from 'esmock'

[tripodsan]

Based on the discussion, my sense is to force the user to import one of these,

I don't think that's the gist of this thread.... I think most are just against px.

from what I understand is:

• keep strict as default
• use esmock.partial(..) if needed (or import esmock from 'esmock/partial )

[iambumblehead]

Based on the discussion, my sense is to force the user to import one of these,

I don't think that's the gist of this thread.... I think most are just against px.

from what I understand is:

* keep `strict` as default

* use `esmock.partial(..)` if needed (or `import esmock from 'esmock/partial` )

Ok, it could be done that way. There's an MR here #139 but tests fail because ts-node doesn't resolve specifiers for './partial' and './strict', TypeStrong/ts-node#1877

I agreed with the points by cawa-93 completely including the import suggestion he made. I did not like gmahomarf's suggestion a choice be made they agree with or "fork or find another project" or implied concerns of release mismanagement.

I hope to close this ticket and be done with this discussion. import esmock from 'esmock' and import esmock from 'esmock/partial are fine. I'll assign the PR to @tripodsan for review when it is ready. Maybe @jakebailey will know why ts-node does not resolve the specifiers?

[jakebailey]

I'm not sure, but I don't know if I have enough info to reproduce it. I'm happy to look if you have some steps.

FWIW I would personally recommend just using named exports instead of trying to use specifiers to split up the API. Otherwise, you're going to have to write a bunch of unique d.ts files for all of these different modules, which is getting unwieldy without writing the code in TS in the first place.

[gmahomarf]

I did not like gmahomarf's suggestion a choice be made they agree with or "fork or find another project" or implied concerns of release mismanagement.

All I meant to say is that some comments in this thread appear to demand a specific way of doing things, which is not how open source is supposed to be. I respect the choices afforded by open source, and that's it. I never meant to say "do what I want or I leave". I actually plan to keep using this library, regardless of what decision comes out of this thread. I spoke in first person in general terms only. I enjoy contributing to libraries I use, regardless of design choices, and this particular library is great for me to use, which is why I contributed.

As for the implications of release mismanagement, I never meant to imply that. Rereading my remarks, they do come off as presumptuous, and I apologize for that.

EDIT: I agree with the named exports suggestion

[iambumblehead]

I want to import esmock this way

import esmock from 'esmock/partial'

rather than this way

import { esmockPartial as esmock } from 'esmock'

Because typescript types files for both should be nearly identical, it might be possible to copy a single typescript file to different names before publishing and running tests. If export specifiers cannot be made to work, lets fallback to the named export

[iambumblehead]

I'm happy to look if you have some steps.

It is understood if you are busy or for any reason can't use time on this. Also, maybe the ts-node developer will respond here TypeStrong/ts-node#1877

To reproduce the issue clone the branch use-separated-export-variants. Use npm install && npm run test:install to fetch all dependencies needed. npm test runs all tests and npm test can also be run from the tests-nodets folder, specifically, to run tests for that folder only

I may be un-available to respond here for the next several hours

[jakebailey]

You could always have:

import { partialMock, strictMock } from "esmock"

const foo = partialMock(...);

If you're trying to make the two methods visually identical and everyone from both parties satisfied. You can also have:

import * as esmock from "esmock";

esmock.partial(...);
esmock.strict(...);

Besides the complexity of specifiers, named exports are a lot more discoverable via features like auto-imports.

[jakebailey]

It is understood if you are busy or for any reason can't use time on this. Also, maybe the ts-node developer will respond here TypeStrong/ts-node#1877

Yeah, I'm pretty busy with other stuff... I'm not sure if the ts-node devs will be able to do anything, as it seems like the error you're seeing is a TS error.

I'll try your repro steps and see what's going on; I was confused because it didn't seem like this repo used ts-node for anything, but I suppose that's one of the test cases? I know that things like ts-node have to use a separate loader mechanism to work, but I don't know if they've reimplemented the ESM algorithm and don't have some specifier support (I would doubt it).

If there's a TS bug here, I can try and reduce it and put it on the TS repo, and see if someone knows what might be going on. (But, I have other stuff going on, of course!)

[iambumblehead]

that's awesome its great you are here thank you -- this issue seems related microsoft/TypeScript#33079

[jakebailey]

Related, yes, though it should all be supported at this point; maybe you're missing a tsconfig with the right options to get TS into module mode, or something.

[cawa-93]

I would personally recommend just using named exports instead of trying to use specifiers to split up the API

It also improve code completition. When some module use default export IDE may not suggest for you "esmock" when you type "es" because, actually, module may be imported with any name. But, if you use named exports, then IDE can index modules and, It will know what module from where may be imported and will suggest you "esmock" when you type "es".

[iambumblehead]

@cawa-93 your responses are always informative and knowledgeable and I want to follow your suggestion. Do you think this is the best way?

import esmock, { esmockPartial, esmockStrict } from 'esmock';

[cawa-93]

@cawa-93 your responses are always informative and knowledgeable and I want to follow your suggestion. Do you think this is the best way?

import esmock, { esmockPartial, esmockStrict } from 'esmock';

I think that, taking into account backward compatibility, this is the optimal option

[jakebailey]

Having everything have the esmock prefix feels redundant to me, but, I guess it really depends on how you're using it.

[iambumblehead]

haha ok this? (feel free to suggest something else)

import esmock, { partial, strict } from 'esmock';

[cawa-93]

haha ok this? (feel free to suggest something else)

import esmock, { partial, strict } from 'esmock';

It's similar to build-in node modules

import path, { resolve as pathResolve, join } from 'node:path'

[iambumblehead]

haha ok this? (feel free to suggest something else)

import esmock, { partial, strict } from 'esmock';

It's similar to build-in node modules

import path, { resolve, join } from 'node:path'

great point

[jakebailey]

Potentially. At least then one could write { partial as esmock } if they chose, or do import * as esmock from "esmock" and then do esmock.partial. I'd prefer the latter, but I'm a sucker for explicitness.

This is bikeshedding, though; all of these are functional, just with their different quirks. The prefix is okay for direct imports.

Note that for the builtin node modules, I don't believe there are any default exports that are callable, e.g. it's (effectively) equivalent to write import path from "path" and import * as path from "path" because they are both just objects; that is where esmock and other libs may differ.

Unless you're doing a major version bump and can break users, you may have to do some odd things to export functions and also add them as properties of the default export to maintain compatibility, like:

const esmock = ...;

export function partial() { ... };

esmock.partial = partial;

export default esmock;

Or something, then write the d.ts for it.

[iambumblehead]

esmock is at version 1.9.8 and we'll include any changes here in a new major version 2.0.0. There are not many users at the moment so backwards compatibility is of lower priority (for me).

This is bikeshedding, through;

This discussion is important because we will define the interface esmock uses from now and if more people begin using esmock, it will be hard to go back and undo or change the decisions made here. Any names or interfaces could technically be used interchangeably, but if we're interested in publishing a package that people like using intuitively the group discussion with different opinions is needed for that.

Let's not use the name "px" if no one likes that. If we know that named exports work better with code analysis tools, lets guide users toward using named exports.

[aladdin-add]

I would slightly want similar apis with proxyquire, is it possible?

at the very least, good to have a migration guide moving from proxyquire. 😄

[iambumblehead]

4 people prefer the proxyquire "partial" style default export: cawa-93, iambumblehead, tripodsan, aladdin-add
2 people prefer the "strict" style default export: jake-bailey, gmahomarf

[iambumblehead]

closing #141