-
Notifications
You must be signed in to change notification settings - Fork 4
Use sjcl as a real dependency #22
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
base: main
Are you sure you want to change the base?
Conversation
This change allows to configure sjcl and use it as a normal dependency, together with @types/sjcl. Impacts on reducing the code of the library.
// 1. If the signature representative s is not between 0 and n - 1, | ||
// output "signature representative out of range" and stop. | ||
if (!s.greaterEquals(new sjcl.bn(0)) || s.greaterEquals(pkS.n) == 1) { | ||
if (!s.greaterEquals(new sjcl.bn(0)) || s.greaterEquals(pkS.n)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be BigNumber
here?
if (!s.greaterEquals(new sjcl.bn(0)) || s.greaterEquals(pkS.n)) { | |
if (!s.greaterEquals(new sjcl.BigNumber(0)) || s.greaterEquals(pkS.n)) { |
EDIT: nevermind, I see the bn()
function is defined in sjcl, and the BigNumber infterface in @types/sjcl.
Although, the greaterEquals()
function also accepts a number
as an argument (https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/sjcl/index.d.ts#L42). Why not use this instead?
if (!s.greaterEquals(new sjcl.bn(0)) || s.greaterEquals(pkS.n)) { | |
if (!s.greaterEquals(0) || s.greaterEquals(pkS.n)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when passing a number, the function internally creates a new object, then it becomes equivalent.
http://bitwiseshiftleft.github.io/sjcl/doc/bn.js.html#line75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall ok.
did you confirm it works when imported?
"clean": "rimraf lib coverage", | ||
"sjcl": "cd node_modules/sjcl && concat -o sjcl.js core/sjcl.js core/aes.js core/bitArray.js core/codecString.js core/codecBase64.js core/codecBytes.js core/sha256.js core/random.js core/bn.js core/exports.js", | ||
"prepack": "npm run sjcl && npm run build", | ||
"prepare": "npm run sjcl && npm run build" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is prepare always run when the library is imported? the issue I faced in the past was @cloudflare/blindrsa-ts
would not work when imported indirectly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could use some help to achieve this.
Either prepare
or postinstall
can trigger the script, however, the path to sjcl needs to be resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there needs to be changes:
prepack
andprepare
run respectively before the folder is packaged, and before publishing respectively (see npm docs). What you're looking for ispostinstall
, because you'd likenode_modules/*
folder to be modified- Folder for
npm
scripts are not fixed. When installed as a dependency, the folder is the one of said dependency, when installed as a main package, it's the main folder. This creates something tricky to trace, not even mentioning having dependency x modify dependency y is a bad practice
If we want to keep @cloudflare/blindrsa-ts unbundled, I suggest:
- Declare
@types/sjcl
as adependency
(not adevDependency
) - Remove scripts
sjcl
,prepare
,pack
The diff is
diff --git a/package-lock.json b/package-lock.json
index fd3a106..b14bc64 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -9,12 +9,12 @@
"version": "0.4.2",
"license": "Apache-2.0",
"dependencies": {
+ "@types/sjcl": "1.0.34",
"sjcl": "1.0.8"
},
"devDependencies": {
"@types/benchmark": "2.1.5",
"@types/jest": "29.5.12",
- "@types/sjcl": "1.0.34",
"@typescript-eslint/eslint-plugin": "7.2.0",
"@typescript-eslint/parser": "7.2.0",
"benchmark": "2.1.4",
@@ -1516,8 +1516,7 @@
"node_modules/@types/sjcl": {
"version": "1.0.34",
"resolved": "https://registry.npmjs.org/@types/sjcl/-/sjcl-1.0.34.tgz",
- "integrity": "sha512-bQHEeK5DTQRunIfQeUMgtpPsNNCcZyQ9MJuAfW1I7iN0LDunTc78Fu17STbLMd7KiEY/g2zHVApippa70h6HoQ==",
- "dev": true
+ "integrity": "sha512-bQHEeK5DTQRunIfQeUMgtpPsNNCcZyQ9MJuAfW1I7iN0LDunTc78Fu17STbLMd7KiEY/g2zHVApippa70h6HoQ=="
},
"node_modules/@types/stack-utils": {
"version": "2.0.1",
diff --git a/package.json b/package.json
index 0121f80..0608410 100644
--- a/package.json
+++ b/package.json
@@ -34,18 +34,15 @@
"format": "prettier './(src|test|examples)/**/!(*.d).ts' --write",
"bench": "tsc -b bench && node ./lib/bench/index.js",
"examples": "tsc -b examples && node ./lib/examples/index.js",
- "clean": "rimraf lib coverage",
- "sjcl": "cd node_modules/sjcl && concat -o sjcl.js core/sjcl.js core/aes.js core/bitArray.js core/codecString.js core/codecBase64.js core/codecBytes.js core/sha256.js core/random.js core/bn.js core/exports.js",
- "prepack": "npm run sjcl && npm run build",
- "prepare": "npm run sjcl && npm run build"
+ "clean": "rimraf lib coverage"
},
"dependencies": {
+ "@types/sjcl": "1.0.34",
"sjcl": "1.0.8"
},
"devDependencies": {
"@types/benchmark": "2.1.5",
"@types/jest": "29.5.12",
- "@types/sjcl": "1.0.34",
"@typescript-eslint/eslint-plugin": "7.2.0",
"@typescript-eslint/parser": "7.2.0",
"benchmark": "2.1.4",
This change allows to configure sjcl and use it as a normal dependency, together with @types/sjcl.
Impacts on reducing the code of the library by half.