connor4312 / cockatiel

🐦 A resilience and fault-handling library. Supports Backoffs, Retries, Circuit Breakers, Timeouts, Bulkhead Isolation, and Fallbacks.
MIT License
1.59k stars 50 forks source link

Advice running unit tests (via node) that import files from this package #77

Closed derekdon closed 1 year ago

derekdon commented 1 year ago

I really like this package... 👍

However, I hit something last night that I wanted to check if anyone else has hit. FWIW I'm pretty sure this isn't going to be a problem with the code within this package itself, but my hope is someone may have hit it / have some ideas.

My specs:

macOS: 12.6
node: v18.16.1
npm: 9.5.1
cockatiel: 3.1.1
typescript: 4.9.5

Context: Running unit tests. We run in both the browser and on node. Browser-based everything is okay, however on the node version, which runs:

uvu -r tsm -r esm -r ./jsdom.setup.ts --tsmconfig ./tsm.js . test.ts

I'm getting the following error due to ??:

/Users/derek/workspace/foundation/node_modules/cockatiel/dist/Policy.js:244
    return new RetryPolicy_1.RetryPolicy({ backoff: opts.backoff || new Backoff_1.ConstantBackoff(0), maxAttempts: opts.maxAttempts ?? Infinity }, new Executor_1.ExecuteWrapper(policy.options.errorFilter, policy.options.resultFilter));
                                                                                                                                     ^

SyntaxError: Invalid or unexpected token
    at Module._extensions..js (node:internal/modules/cjs/loader:1310:10)
    at Object.loader (/Users/derek/workspace/foundation/node_modules/tsm/require.js:1)
npm ERR! Lifecycle script `test` failed with error: 
npm ERR! Error: command failed 
npm ERR!   in workspace: @foundation/machine@14.67.4 
npm ERR!   at location: /Users/derek/workspace/foundation/packages/machine 

Process finished with exit code 1

It seems like something in my chain doesn't like the ?? on https://github.com/connor4312/cockatiel/blob/master/src/Policy.ts#L425

It's weird because we run a ton of tests that use third-party deps, many of which I'm sure use ?? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing.

I suspect it's an issue with the tsconfigs, compilerOptions, libs etc. I'll keep digging.

derekdon commented 1 year ago

Interestingly, if I add "type": "module", directly to the package in node_modules it runs in the node test flow fine.

{
  "name": "cockatiel",
  "version": "3.1.1",
  "description": "A resilience and transient-fault-handling library that allows developers to express policies such as Backoff, Retry, Circuit Breaker, Timeout, Bulkhead Isolation, and Fallback in a fluent and thread-safe manner. Inspired by .NET Polly.",
  "main": "dist/index.js",
  "module": "dist/esm/index.js",
  "engines": {
    "node": ">=16"
  },
  "type": "module", // < testing

This makes me wonder if my tooling doesn't like the presence of the module property which wasn't officially supported... I believe exports replaces it. I'll keep digging...

connor4312 commented 1 year ago

node: v18.16.1

I don't think this is right. As the MDN page says, null coalescing has been available since Node 14 (released 3 yrs ago), and I personally have many Node 16 applications which are using this package.

➜  ~ node --version
v14.21.3
➜  ~ node -p "undefined ?? true"
true

I think something in your stack of loaders (tsm? esm? uvu?) is using a parser that's quite old. This doesn't seem to be an issue with this module.

derekdon commented 1 year ago

@connor4312 thanks for your response. As you pointed out, my loader chain has a few layers so it's been difficult to debug.

I used overrides to ensure the internal esbuild of these was the very latest (as were the packages tsm and esm themselves)... but didn't make a difference.

The node version running my test process is +18, and I assume this isn't changing somehow during the execution.

I suspect the issue is how esm in particular is handling the cockatiel package.

Noticed setting mode to strict in esm config works for my use case.

Screenshot 2023-08-15 at 12 03 32

Not that it's really something I want to do as I suspect will bite me in the future regarding other deps the package may introduce...

Seems like others have hit similar elsewhere: https://github.com/vercel/ncc/issues/889, https://github.com/standard-things/esm/issues/909, https://github.com/standard-things/esm/issues/866

derekdon commented 1 year ago

So further update, the issue seems to be esm. It's using a really old version of the acorn parser, and it's baked in rather than something we can override. In fact, esm itself seems pretty outdated now.

For anyone else that finds themselves here, this script works with this package fine.

"test": "uvu -r tsm --tsmconfig ./tsm.js -r @esbuild-kit/cjs-loader -r ./jsdom.setup.ts . test.ts",

Here all I've done is swap out esm for @esbuild-kit/cjs-loader.

Sorry for the noise @connor4312.