Skip to content
This repository has been archived by the owner on Jan 11, 2022. It is now read-only.

Fix karma, readme updates, combine source to one file #109

Merged
merged 22 commits into from
Jan 10, 2022

Conversation

ryanio
Copy link
Contributor

@ryanio ryanio commented Jan 9, 2022

This PR updates the karma-typescript dependency to a built version from master using gitpkg for the unreleased fix.

It also includes some refactoring and readme updates.

@coveralls
Copy link

coveralls commented Jan 9, 2022

Coverage Status

Coverage increased (+0.9%) to 96.032% when pulling 054fa9a on fix-karma-and-other-updates into 7004854 on master.

@paulmillr
Copy link
Member

paulmillr commented Jan 9, 2022

I know using 3 files instead of 1 is much more readable, just not sure about ESM support - or Deno support.

I have been making my stuff work with those, and there are some things that you need to take into account.

For example, in Deno you can't import .js files, and you need to specify extensions in imports. In ESM, you cannot import .ts files.

My suggestion would be to move everything into one file and place it in root. After that, after the release, we could work on .esm tsconfig and Deno package. I could help here.

I have worked around "many files" thing in bls12-381 by using export maps but we definitely don't want to force Deno users to do that (a separate .json file)

ryanio added 6 commits January 8, 2022 18:41
…th browserify and was able to RLP encode in the browser from dist/index.js
  - use npm 7 for github url integrity support (for karma-typescript unreleased workaround)
  - run coverage inside node v16 workflow so a duplicative job is not needed
@ryanio ryanio force-pushed the fix-karma-and-other-updates branch from dd03799 to ab338ad Compare January 9, 2022 03:30
@ryanio
Copy link
Contributor Author

ryanio commented Jan 9, 2022

@paulmillr sounds good, thanks for the feedback, we already had 2 files (index, types) so I thought a third for utils would be harmless (the improved readability is great), but I am also in favor of broader ecosystem support, so whatever we need to do here - if it is one file then that's fine by me too.

Appreciate your help with esm and Deno insights, we can probably bring these learnings over to our monorepo as well.

@ryanio
Copy link
Contributor Author

ryanio commented Jan 9, 2022

For the karma-typescript fix, I wrote some of my findings in this comment: ethereumjs/ethereumjs-monorepo#1602 (comment)

@ryanio ryanio changed the title Fix karma, readme updates, move utils to own file Fix karma, readme updates, combine source to one file Jan 9, 2022
@ryanio
Copy link
Contributor Author

ryanio commented Jan 9, 2022

@talentlessguy has helped us with Deno compatibility in the past, let us know how this PR is looking

@ryanio ryanio linked an issue Jan 9, 2022 that may be closed by this pull request
Copy link
Member

@paulmillr paulmillr left a comment

Choose a reason for hiding this comment

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

Looks great to me!

.github/workflows/build.yml Outdated Show resolved Hide resolved
@ryanio
Copy link
Contributor Author

ryanio commented Jan 10, 2022

thanks!

@ryanio ryanio merged commit e4b371d into master Jan 10, 2022
@ryanio ryanio deleted the fix-karma-and-other-updates branch January 10, 2022 02:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve type definitions for decode
3 participants