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

Revisit no-floating-promises rule in tests #16

Closed
oskardudycz opened this issue Feb 19, 2024 · 0 comments · Fixed by #61
Closed

Revisit no-floating-promises rule in tests #16

oskardudycz opened this issue Feb 19, 2024 · 0 comments · Fixed by #61
Labels
bug Something isn't working testing Up for grabs
Milestone

Comments

@oskardudycz
Copy link
Collaborator

Currently, because of nodejs/node#51292, the eslint rule is broken. I disabled it in the test files, but would you like to reenable it once that's somehow solved. See also typescript-eslint/typescript-eslint#8433.

oskardudycz added a commit that referenced this issue Apr 28, 2024
…in bundled Emmett code

The issue is that `esbuild` (for `commonjs`) stripes the 'node:' prefix for targets different from the node (or using node earlier than 14) as they cannot handle packages with prefix like `node:test`. See more in:
evanw/esbuild@6b4f970

`Tsup` that I'm using for bundling also seems not to be handling that fully as it's built on top of `esbuild`: egoist/tsup#1003

As there's no dependency to `test` package, then bundled code fails to load in commonjs environment. One solution is to add dependency to shim, as @alex-laycalvert suggested in #60. But I'm reluctant to do it, as I don't want to target older node environments than Node LTS version (currently >= 20.11.1). I also don't want to add dependency on the unmaintained shim package with potential vulnerabilities.

I could try to change tsconfig target from ESNext to Node, but at the moment I don't want to have this limitation, as I'd like to have the main Emmett package working both with backend and frontend.

The change removes the event store matrix tests, as they don't need to be exposed now. They're still available as js map files to make internal referencing work correctly.

This change should be seen more as a fix rather than solving the root cause. I need to think longer on alternative solutions like having internal project for testing event stores and not having this test at all.

Fixes #59,
Closes #60

Besides that, following the boy scout rule I removed the `no-floating-promise` ingores as I found the way of handling them using `void` keyword before describes and it. And that Fixes #16

@alex-laycalvert @thiagomini @mkubasz FYI
@oskardudycz oskardudycz added this to the 0.7.1 milestone Apr 28, 2024
@oskardudycz oskardudycz added bug Something isn't working testing labels Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working testing Up for grabs
Projects
None yet
1 participant