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

Generate ESM code and use vitest for tests #172

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Conversation

tstirrat15
Copy link
Contributor

@tstirrat15 tstirrat15 commented Oct 25, 2024

Fixes #171

Description

This started out as a QOL thing. Vitest is the current state-of-the-art and supports typescript out of the box without the need for manual transpilation. It also is quite fast and supports concurrency and parallelization nicely.

However, Vite doesn't play nicely in non-ESM environments, and we weren't generating ESM with our TS configuration. This updates that, since we don't have a reason not to - Node has supported ESM since version 16.

Changes

Testing

Review. See that things are green.

@tstirrat15 tstirrat15 requested a review from a team October 25, 2024 00:06
@tstirrat15 tstirrat15 requested a review from a team as a code owner January 17, 2025 19:02
@tstirrat15 tstirrat15 changed the title Use vitest for tests Generate ESM code and use vitest for tests Jan 17, 2025
Copy link
Contributor Author

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See annotations

Comment on lines -3 to -4
src/authzedapi/authzed/api/v0
src/authzedapi/authzed/api/v1alpha1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer necessary with the changes to the buf generation file.

Comment on lines +13 to +14
# NOTE: This grabs only the v1 proto and ignores v0 and v1dev.
- "authzed/api/v1"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These being those changes

@@ -2,6 +2,7 @@
"name": "@authzed/authzed-js-node",
"author": "authzed",
"version": "0.19.0",
"type": "module",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make importing code happy with the ES code found in this module.

"only-run-tests": "ts-node node_modules/jasmine/bin/jasmine --config=jasmine.json",
"buf": "./buf.gen.yaml",
"only-run-tests": "vitest",
"buf": "buf generate && tsc-esm-fix --src src/authzedapi --ext='.js'",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the tsc-esm-fix is needed until timostamm/protobuf-ts#656 is fixed.

"lint": "./node_modules/.bin/eslint src",
"build": "tsc",
"prepublish": "yarn build",
"build-js-client": "tsc --target es2018 --declaration false --outDir js-dist"
"build-js-client": "tsc --declaration false --outDir js-dist"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to override target here - they can use the same targets.

Comment on lines +1 to +4
import { ClientSecurity } from "./util.js";
import * as v1 from "./v1.js";
import { Consistency } from "./v1.js";
import { generateTestToken } from './__utils__/helpers.js'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typescript is sticky on this point - if you're using node16 as the module target, you need to have a file suffix on your relative imports. Apparently this is a part of the JS spec that TS implements but it mostly annoys people.

I ran tsc-esm-fix to generate these changes.


describe("a check following a write of schema and relationships", () => {
it("should succeed", (done) => {
it("should succeed", () => new Promise<void>((done) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a drive-by TS fix.

Comment on lines +2 to +3
"extends": "@tsconfig/node18/tsconfig.json",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually probably the biggest change - this extends a TS config that works with Node 18. When we walk our node support we can change this and get the new configuration easily.

I removed all of the configuration that would re-specify what's already in the extension.

Copy link
Contributor

@alecmerdler alecmerdler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet, great work!

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

Successfully merging this pull request may close these issues.

Use Vitest instead of Jasmine
2 participants