Skip to content

Conversation

@tegefaulkes
Copy link
Contributor

@tegefaulkes tegefaulkes commented Apr 3, 2025

Description

This PR addresses converting Polykey-CLI to ESM.

Issues Fixed

Tasks

  • 1. Migrate to ESM.
  • 2. Check that Nix build works.
  • 3. Check that the docker image runs.

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@tegefaulkes tegefaulkes self-assigned this Apr 3, 2025
@tegefaulkes
Copy link
Contributor Author

I have a functional bundled code now. Right now I'm running into problems with tests. Specifically mocking. It seems that how mocks of modules and imports are handled are changed with ESM. There is a new major constraint causing problems.

  1. The ESM imports are read-only now. So we can't do any cheeky overriding of functionality globally.
  2. Its possible to mock modules that are imported. HOWEVER you must mock then before they are imported. Since most imports are static and done before any code execution these can't be mocked. Only dynamically imported code can be mocked now.

So the usual methods can't be done anymore. I need to review and think of alternative ways to handle mocking.

@CMCDragonkai
Copy link
Member

Amy mentioned that mocking doesn't work the same way as before with ESM, when she was working on js-ws and js-rpc.

Generally speaking mocking isn't the best way to do testing. DI is preferred in most cases. Mocking is a hack, for very static scenarios, you're reinjecting dynamism when it was supposed to be static.

In CJS, it was dynamic masquerading as static, but in ESM, it's static properly now.

Can you give a list of locations where mocking is a problem, you can use R1 to track and list all locations with a description after indexing the codebase, as well as a fuzzy search.

Then in most cases we should be able to convert to DI instead. And other cases, we may just do a proper integration.

@tegefaulkes
Copy link
Contributor Author

The problem here is we have a clear boundary of the cli running as its own process or pretending to in some cases. We can't DI across that boundary.

The options here are

  1. Remove all mocks
  2. Special case testing code with flags.
  3. Update code that needs to be mocked with dynamic imports so mocks can be applied before running them.

Given the CLI already depends on dynamic importing for optimisation then option 3 seems the least annoying.

@linear
Copy link

linear bot commented Apr 15, 2025

ENG-556

@tegefaulkes
Copy link
Contributor Author

Everything is fixed up now. I just need to check the nix build works and the docker image works.

@tegefaulkes tegefaulkes force-pushed the feature-eng-556-polykey-cli-migration-to-esm branch from afd92f9 to 0ea0a1b Compare April 16, 2025 02:36
@tegefaulkes tegefaulkes marked this pull request as ready for review April 16, 2025 02:55
@tegefaulkes tegefaulkes merged commit ddd5a2b into staging Apr 16, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Polykey-CLI: Migration to ESM

3 participants