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

lib: allow mocking primordials with --expose-internals #40733

Closed

Conversation

benjamingr
Copy link
Member

This pull request allows swapping primordials when --expose-interals is passed. This is required for libraries like SinonJS to mock Date.

I am not sure why primordials was frozen in the first place since the object is not user-accessible as far as I'm aware.

cc @aduh95 @fatso83

Refs: sinonjs/fake-timers#344
Refs: #36872

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 5, 2021
@benjamingr
Copy link
Member Author

Initially I thought to optionally do this only if --expose-internals is passed but that seemed pointless since it's not accessible otherwise.

I'm not sure how to test this (similar PRs did not contain tests) but I can add a test with --expose-internals that checks you can set primordials.Date if that helps.

@fatso83
Copy link

fatso83 commented Nov 5, 2021

Ooh, this would be super useful for Node > 15.7 where the @sinonjs/fake-timers implementations used by Jest and Sinon is not able to do touch the Date implementation used by Node internals like http.

@addaleax
Copy link
Member

addaleax commented Nov 5, 2021

I think the real question here is what value we get out of always reading the primordial Date instead of the global Date to begin with.

@targos
Copy link
Member

targos commented Nov 6, 2021

--expose-internals is not a public API. I'm pretty worried that popular libraries think about using it

@fatso83
Copy link

fatso83 commented Nov 6, 2021

As they say about API design; make the common things easy and the difficult things possible. Currently, a certain use case is no longer possible, and this would make it possible for the ones that cared for it.

@targos
Copy link
Member

targos commented Nov 6, 2021

I'm not saying we shouldn't make it possible. I'm saying that making it possible through --expose-internals is not the right way.

@aduh95
Copy link
Contributor

aduh95 commented Nov 6, 2021

Note that mutating primordials is not necessary to achieve what is being discussed in the other issue for someone daring enough to use --expose-internals:

require('internal/http').utcDate = function utcDate() {
  return 'custom string';
};

const http = require('http');

server = http.createServer((req, res) => {
  req.on('data', () => void null);
  req.on('end', () => {
    res.writeHead(200);
    res.end();
  });
});
server.listen(8008, () => {
  const opts = {
    hostname: 'localhost',
    method: 'GET',
    path: '/',
    port: 8008,
    family: 4,
  };
  const req = http.request(opts, (res) => {
    res.on('data', () => void null);
    res.on('end', () => {
      console.log(res.headers.date === 'custom string');
    });
  });
  req.end();
});

@benjamingr
Copy link
Member Author

benjamingr commented Nov 8, 2021

@fatso83

What if we just mock .utcDate if --expose-internals is passed?

@fatso83
Copy link

fatso83 commented Nov 8, 2021

@benjamingr Yeah, I saw that was possible to begin with, but it would not allow us to fully control Date and all that jazz. But then maybe they need to employ DI or something to avoid the http internals at all. Good enough for now, I guess :)

@targos
Copy link
Member

targos commented Nov 9, 2021

I suggest we close this PR and instead open a feature request issue with a description of what API you would like to have (with the goal of not relying on monkey-patching to change the behavior of Node internals).

@benjamingr
Copy link
Member Author

I don't think discussion has died down - but I am not set on this particular solution.

The ask is to be able to mock dates (internally) - one (easy) way to achieve this is to be able to override primordials.Date - this is for testing only basically.

@targos
Copy link
Member

targos commented Nov 9, 2021

one (easy) way to achieve this is to be able to override primordials.Date

This assumes that Node.js uses the Date object internally, which means depending on an implementation detail (we could decide at any point to call into libuv or some other C++ binding directly)

@benjamingr
Copy link
Member Author

It does, and in general mocking libraries like fake-timers work under the assumption that things like this can/may break.

It also "breaks" whenever node adds additional properties to timers and the sinon code would have to be adjusted in those cases.

We can choose to just not enable that use case (it's not a timer, sinon only mocks a few properties etc) and allow passing a "start time" or similar to Node.js or suggest the user mocks system time instead for example?

@benjamingr benjamingr closed this Jan 16, 2022
@fatso83
Copy link

fatso83 commented Jan 19, 2022

@benjamingr Did an alternative suggestion come up?

@benjamingr
Copy link
Member Author

@fatso83 no, this just stalled and people haven't asked a lot for this before and sinon never made the guarantee to allow mocking node internal usage of Date (which may be replaced with C++ anyway code in which case it won't help).

I can work on a flag for Node.js that allows setting the data on http headers but it's not a common ask

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants