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.

Sign up for GitHub

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

Jump to bottom

feat: add tests workflow for deno #423

Open
wants to merge 14 commits into
base: feature/deno
Choose a base branch
from
Open

feat: add tests workflow for deno #423

wants to merge 14 commits into from

Conversation

nazarhussain
Copy link
Contributor

Motivation

Make the packages compatible with Deno.

Description

  • Make unit tests compatible with Deno
  • Make spec tests compatible with Deno

Steps to test or reproduce

  • Run all tests

@nazarhussain nazarhussain changed the title Add tests workflow for deno chore: add tests workflow for deno Nov 21, 2024
@nazarhussain nazarhussain changed the title chore: add tests workflow for deno feat: add tests workflow for deno Nov 21, 2024
@nazarhussain nazarhussain marked this pull request as ready for review November 21, 2024 13:54
@nazarhussain nazarhussain requested a review from a team as a code owner November 21, 2024 13:54
@nazarhussain nazarhussain self-assigned this Nov 22, 2024
twoeths
twoeths reviewed Nov 25, 2024
View reviewed changes
packages/persistent-merkle-tree/src/hasher/util.ts
@@ -1,4 +1,4 @@
import {byteArrayToHashObject, HashObject, hashObjectToByteArray} from "@chainsafe/as-sha256/lib/hashObject";
import {byteArrayToHashObject, HashObject, hashObjectToByteArray} from "@chainsafe/as-sha256";
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@twoeths
Copy link
Contributor

twoeths commented Nov 25, 2024

while I see the improvement on the source code, like not to import lib from other modules, I don't see why do we have to pick Deno and make sure lodestar to be compliant to it? there are a lot of deno specific resources added in this PR

I guess we need to put this on hold until we have final approach on which javascript runtime that lodestar want to experiment ChainSafe/lodestar#7237

@nazarhussain
Copy link
Contributor Author

nazarhussain commented Nov 25, 2024 •
edited
Loading

while I see the improvement on the source code, like not to import lib from other modules, I don't see why do we have to pick Deno and make sure lodestar to be compliant to it? there are a lot of deno specific resources added in this PR

Deno is needed/recommended to publish packages to jsr.io which is intention to test with deno first in this PR.

And here is the details that why I am moving to support jsr.io https://jsr.io/docs/why

@twoeths
Copy link
Contributor

twoeths commented Nov 26, 2024

Does Bun support jsr.io?

if yes I'd go with Bun as we'll most likely to go with it. Again, need to wait for the final decision on ChainSafe/lodestar#7237

if both NodeJS + Bun don't support jsr.io, only Deno does then we need to see if it's a must to support jsr.io. I found it weird to target Bun/NodeJS while having Deno specific resources in the repo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twoeths twoeths twoeths left review comments

Assignees

@nazarhussain nazarhussain

Labels
as-sha256 CI persistent-merkle-tree persistent-ts simpleserialize.com ssz
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nazarhussain @twoeths