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

Compability changes #11

Closed
wants to merge 2 commits into from
Closed

Conversation

lgarron
Copy link
Contributor

@lgarron lgarron commented Nov 3, 2023

  • Import the sodium object in a single place and use the object across the project. This simplifies the import semantics and enables compatibility with node and bun: Incompatibilities with node and bun #9
  • Configure CI to run using GitHub Actions.
  • Add the examples from the README to tests/readme-examples and test them in CI using bun.
  • Add npm run clean
  • Update tsconfig.json to avoid warnings:
    • Set "module": "nodenext" to avoid: "Option 'module' must be set to 'NodeNext' when option 'moduleResolution' is set to 'NodeNext'.ts" (from TypeScript).
    • Set the "include" to the "lib" folder so that the top-level await lines in tests/readme-examples don't show errors.

- Import the `sodium` object in a single place and use it across the object. This simplifies the import semantics and enables compatibility with `node` and `bun`: FiloSottile#9
- Configure CI to run using GitHub Actions.
- Add the examples from the README to `tests/readme-examples` and test them in CI using `bun`.
- Add `npm run clean`
- Update `tsconfig.json` to avoid warnings:
  - Set `"module": "nodenext"` to avoid: "Option 'module' must be set to 'NodeNext' when option 'moduleResolution' is set to 'NodeNext'.ts" (from TypeScript).
  - Set the `"include"` to the `"lib"` folder so that the top-level await lines in `tests/readme-examples` don't show errors.
@lgarron
Copy link
Contributor Author

lgarron commented Nov 3, 2023

@FiloSottile: Apologies for the omnibus PR. I've tried to keep with the existing project conventions where possible, but if you're interested in a subset of changes I'd be happy to break it apart.

I've tested that the resulting build works in node, bun, and browsers in a real-world project I'm working on.

@lgarron
Copy link
Contributor Author

lgarron commented Nov 3, 2023

Hm, I'm sorry, there seems to be an issue with this that only happens in CI but which I can't reproduce locally:

RUN  v0.34.1 /home/runner/work/age.ts/age.ts

✓ tests/index.test.ts (10 tests) 59ms
✓ tests/testkit.test.ts (109 tests) 117ms
✓ tests/property.test.ts (4 tests) 262ms
❯ tests/format.test.ts (6 tests | 4 failed) 30ms
❯ tests/format.test.ts > parseHeader > should parse a well formatted header
→ Cannot read properties of undefined (reading '_malloc')
❯ tests/format.test.ts > parseHeader > should reencode to the original header
→ Cannot read properties of undefined (reading '_malloc')
❯ tests/format.test.ts > decodeBase64 > should parse a valid base64 string
→ Cannot read properties of undefined (reading '_malloc')
❯ tests/format.test.ts > decodeBase64 > should parse a valid base64 string with spare bits
→ Cannot read properties of undefined (reading '_malloc')
✓ tests/hkdf.test.ts (3 tests) 23ms

⎯⎯⎯⎯⎯⎯⎯ Failed Tests 4 ⎯⎯⎯⎯⎯⎯⎯

FAIL tests/format.test.ts > parseHeader > should parse a well formatted header
TypeError: Cannot read properties of undefined (reading '_malloc')
❯ v node_modules/libsodium-wrappers-sumo/dist/modules-sumo/libsodium-wrappers.js:1:17642
❯ new u node_modules/libsodium-wrappers-sumo/dist/modules-sumo/libsodium-wrappers.js:1:17556
❯ Object.e.from_base64 node_modules/libsodium-wrappers-sumo/dist/modules-sumo/libsodium-wrappers.js:1:80870
❯ decodeBase64 lib/format.ts:3:51
1| import { sodium } from "./sodium.js";
2|
3| export const decodeBase64 = (s: string) => sodium.from_base64(s, sodiu…
| ^
4| export const encodeBase64 = (s: Uint8Array) => sodium.to_base64(s, sod…
5|
❯ parseNextStanza lib/format.ts:78:22
❯ Module.parseHeader lib/format.ts:116:21
❯ tests/format.test.ts:14:19

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/4]⎯

FAIL tests/format.test.ts > parseHeader > should reencode to the original header
Test Files 1 failed | 4 passed (5)
TypeError: Cannot read properties of undefined (reading '_malloc')
Tests 4 failed | 128 passed (132)
❯ v node_modules/libsodium-wrappers-sumo/dist/modules-sumo/libsodium-wrappers.js:1:17642
Start at 04:52:56
❯ new u node_modules/libsodium-wrappers-sumo/dist/modules-sumo/libsodium-wrappers.js:1:17556
Duration 1.15s (transform 163ms, setup 0ms, collect 1.02s, tests 491ms, environment 1ms, prepare 605ms)
❯ Object.e.from_base64 node_modules/libsodium-wrappers-sumo/dist/modules-sumo/libsodium-wrappers.js:1:80870

❯ decodeBase64 lib/format.ts:3:51
1| import { sodium } from "./sodium.js";
2|
3| export const decodeBase64 = (s: string) => sodium.from_base64(s, sodiu…
| ^
4| export const encodeBase64 = (s: Uint8Array) => sodium.to_base64(s, sod…
5|
❯ parseNextStanza lib/format.ts:78:22
❯ Module.parseHeader lib/format.ts:116:21
❯ tests/format.test.ts:22:19

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[2/4]⎯

FAIL tests/format.test.ts > decodeBase64 > should parse a valid base64 string
TypeError: Cannot read properties of undefined (reading '_malloc')
❯ v node_modules/libsodium-wrappers-sumo/dist/modules-sumo/libsodium-wrappers.js:1:17642
❯ new u node_modules/libsodium-wrappers-sumo/dist/modules-sumo/libsodium-wrappers.js:1:17556
❯ Object.e.from_base64 node_modules/libsodium-wrappers-sumo/dist/modules-sumo/libsodium-wrappers.js:1:80870
❯ Module.decodeBase64 lib/format.ts:3:51
1| import { sodium } from "./sodium.js";
2|
3| export const decodeBase64 = (s: string) => sodium.from_base64(s, sodiu…
| ^
4| export const encodeBase64 = (s: Uint8Array) => sodium.to_base64(s, sod…
5|
❯ tests/format.test.ts:31:26

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[3/4]⎯

FAIL tests/format.test.ts > decodeBase64 > should parse a valid base64 string with spare bits
TypeError: Cannot read properties of undefined (reading '_malloc')
❯ v node_modules/libsodium-wrappers-sumo/dist/modules-sumo/libsodium-wrappers.js:1:17642
❯ new u node_modules/libsodium-wrappers-sumo/dist/modules-sumo/libsodium-wrappers.js:1:17556
❯ Object.e.from_base64 node_modules/libsodium-wrappers-sumo/dist/modules-sumo/libsodium-wrappers.js:1:80870
❯ Module.decodeBase64 lib/format.ts:3:51
1| import { sodium } from "./sodium.js";
2|
3| export const decodeBase64 = (s: string) => sodium.from_base64(s, sodiu…
| ^
4| export const encodeBase64 = (s: Uint8Array) => sodium.to_base64(s, sod…
5|
❯ tests/format.test.ts:34:26

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[4/4]⎯

Error: Process completed with exit code 1.

I don't yet know if that's due to code changes in this PR, or just from trying to run libsodium-wrappers-sumo in the GitHub Actions runners.

@lgarron lgarron mentioned this pull request Nov 3, 2023
@FiloSottile
Copy link
Owner

Thank you! I had a few of these changes staged but got stuck on trying to write a test for esbuild compatibility. It looks like partially due to libsodium choices the esbuild bundle runs in the browser but not in node, so to test it I'll have to run puppeteer or something like that.

@lgarron
Copy link
Contributor Author

lgarron commented Nov 3, 2023

Got it!

The tests needed the same initialization conventions as well: lgarron@efadd20

(But I still really wish top-level await were ready for use in libraries, that would make this all much simpler.)

@lgarron
Copy link
Contributor Author

lgarron commented Nov 3, 2023

That said, running in Puppeteer would probably still be smart.

FiloSottile added a commit that referenced this pull request Nov 3, 2023
Fixes Node module resolution.

Partially based on #11 by @lgarron.

Fixes #9
This was referenced Nov 3, 2023
FiloSottile added a commit that referenced this pull request Nov 3, 2023
Fixes Node module resolution.

Partially based on #11 by @lgarron.

Fixes #9
@FiloSottile
Copy link
Owner

Closing in favor of #14. Thanks again!

@FiloSottile FiloSottile closed this Nov 3, 2023
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.

2 participants