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

Refactor tstyche#395 #1

Open
rickosborne opened this issue Jan 4, 2025 · 4 comments
Open

Refactor tstyche#395 #1

rickosborne opened this issue Jan 4, 2025 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@rickosborne
Copy link
Collaborator

Before we can get this to build, we'll need to get the TSTyche API to expose a few things. Most of this is in tstyche/tstyche#395 already.

Add:

  • Expose types via tstyche/tstyche for CJS (also)
  • Expose Runner.addReporter

Keep:

  • Refactors to *Result types

Remove:

  • as-mocha code
@rickosborne rickosborne added the enhancement New feature or request label Jan 4, 2025
@rickosborne rickosborne self-assigned this Jan 4, 2025
@mrazauskas
Copy link
Member

mrazauskas commented Jan 4, 2025

Expose types via tstyche/tstyche for CJS (also)

Is that a hard requirement? Does this mean I should ship TSTyche as dual CJS/ESM package. Hm.. I was trying to avoid that. Could we try await import("tstyche/tstyche"), perhaps? That’s what I do in my VS Code extension draft.

Expose Runner.addReporter

There is EventEmitter.addReporter(). I was thinking to add Runner.addReporter() while shaping up reporters implementation, but decide to avoid having to similar APIs. Internally each domain (like cli or runner) is responsible to clean up their reporters / handlers. They are free to decide at which stage to do that. Having Runner.addReporter() would mean that the runner is delegated the clean up task as well. Which actually is not always what you want.

That’s why I decided to keep only EventEmitter.addReporter() which gives more powers (and more responsibilities) the outside users:

const eventEmitter = new EventEmitter();

eventEmitter.addReporter(firstReporter, secondReporter);

// do something

eventEmitter.removeReporter(secondReporter);

// do something more

eventEmitter.removeReporters();

There are also handlers. The difference is that handlers are called before reporters, so they can modify Result* which later passed to reporters. Even more power (;

@rickosborne
Copy link
Collaborator Author

Expose types via tstyche/tstyche for CJS (also)

Is that a hard requirement? Does this mean I should ship TSTyche as dual CJS/ESM package.

I don't think you'd need a full CJS build. I think it may be enough to alter the TSTyche package.json a bit to clarify what you already have:

  "exports": {
      ".": {
+       "types": "./build/index.d.ts",
        "import": "./build/index.js",
        "require": "./build/index.cjs"
      },
      "./package.json": "./package.json",
-     "./tstyche": "./build/tstyche.js"
+     "./tstyche": {
+       "types": "./build/tstyche.d.ts",
+       "import": "./build/tstyche.js",
+     }
    }
  },

The problem I'm working to solve is getting a CJS build of as-mocha running, which right now fails because:

yarn run build:cjs

source/mocha.ts:5:65 - error TS2307: Cannot find module 'tstyche/tstyche' or its corresponding type declarations.
  There are types at 'node_modules/tstyche/build/tstyche.d.ts', but this result could not be resolved under your current 'moduleResolution' setting. Consider updating to 'node16', 'nodenext', or 'bundler'.

But, of course, I can't update moduleResolution if I want to publish CJS, as it must be "Node":

{
  "compilerOptions": {
    "module": "CommonJS",
    "moduleResolution": "Node",
    "noEmit": false,
    "outDir": "./build/cjs",
    "target": "ES2016"
  },
  "include": ["./source/mocha.ts"],
  "extends": "./tsconfig.json"
}

@rickosborne
Copy link
Collaborator Author

As for the Reporter thing ... That may be leftover thinking from my initial approach. It looks like you already have the code in place to load a reporter from an imported file. So I think I can just alter the as-mocha build a bit to produce that reporter as its own artifact and then use it that way. I'll give it a shot.

@mrazauskas
Copy link
Member

If I run yarn tsc --project tsconfig.cjs.json it emits an error, but the project is still build. Line 15 in ./build/cjs/source/mocha.js looks like this:

const tstyche_1 = require("tstyche/tstyche");

If you run this in Node.js, it will blows up with:

Support for loading ES Module in require() is an experimental feature and might change at any time

Is this expected? In this case "moduleResolution": "node10" does not look right. Might be worth waiting for --module node20 from microsoft/TypeScript#60761.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants