evanderkoogh / otel-cf-workers

An OpenTelemetry compatible library for instrumenting and exporting traces for Cloudflare Workers
BSD 3-Clause "New" or "Revised" License
210 stars 40 forks source link

Error loading `ts-checked-fsm` when using vitest and miniflare #48

Closed rdooley closed 2 months ago

rdooley commented 10 months ago

Testing an app using vitest with environment: 'miniflare' getting the following error from the ts-checked-fsm dependency

SyntaxError: Named export 'stateMachine' not found. The requested module 'ts-checked-fsm' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'ts-checked-fsm';
const { stateMachine } = pkg;
evanderkoogh commented 10 months ago

Aarrrgghh... At what point do I fork that library to make it an ESM thing? :D

Would you be able to give me a repo that reproduces this? Doing what the error message suggests doesn't work because the module doesn't have a default export?

rdooley commented 10 months ago

Aarrrgghh... At what point do I fork that library to make it an ESM thing? :D

Great question

Would you be able to give me a repo that reproduces this? Doing what the error message suggests doesn't work because the module doesn't have a default export?

I can say that importing instrument like this in a test, or a module imported by a test

import {instrument} from '@microlabs/otel-cf-workers';

with a vitest config of like

/// <reference types="vitest" />
/// <reference types="vite/client" />

import {defineConfig} from 'vite';
import react from '@vitejs/plugin-react';
import tsconfigPaths from 'vite-tsconfig-paths';

export default defineConfig({
  plugins: [react(), tsconfigPaths()],
  test: {
    silent: false, // display test logs in the console
    globals: true,
    environment: 'happy-dom', // fails with all of them including the workerd one
    exclude: [
      'test/playwright/**',
      '**/node_modules/**',
      'node_modules',
      'build',
    ],
  },
} as any);

will reproduce

Not a horrible bug for right now, easy workaround is just don't import instrument in tests.

evanderkoogh commented 10 months ago

Yeah. I wouldn’t necessarily recommend people instrument their tests anyway, but a lot of people are going to do/want to do it.

jesseditson commented 8 months ago

Hey, this lib seems super helpful, but I have a pretty large integration test suite written in workers 2.0 - in the latest workers API, there is no environment-level env, and I the way I had this lib set up was to wrap the root export with instrument - both of these things seem to make it quite a challenge to conditionally import this lib.

Curious if the suggested resolution in this issue is using the global workers API or if I'm missing something in terms of how a conditional import might be achievable.

rdooley commented 8 months ago

@jesseditson I ended up putting the handler we tested in a separate file from the file registered in the main of the wrangler.toml, which ends up looking something like

import {handler} from './indexHandler';

const configFn = ...REDACTED BITS...
export default instrument(handler, configFn);
jesseditson commented 8 months ago

Interesting - any chance you use instrumentDO? Certainly seems possible to use this approach but it will result in a lot of shared boilerplate and quite a large divergence in test & prod. Thank you!

rdooley commented 8 months ago

the app in question doesn't use Durable Objects, but I strongly suspect a similar approach would work without adding code duplication You could have a index like the worker example and then the handleDO in a separate handler file which is imported by both the index and your tests

evanderkoogh commented 8 months ago

That is absolutely what I would recommend everyone do indeed. Export the "real" handler in one file and then in your index file do all the setup required for the instrumentation. Not only does it help with tests, but it also keeps your instrumentation configuration and setup away from your production code.

It is still a thing I am trying to solve, but I might just do that by forking the library.. I am just not proficient enough with CommonJS/ES modules stuff, especially in node environments etc.