-
Notifications
You must be signed in to change notification settings - Fork 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
[fix]: update configuration for exporting package #25
[fix]: update configuration for exporting package #25
Conversation
"repository": { | ||
"type": "git", | ||
"url": "https://github.com/jpmorganchase/onyx-ssi-sdk" | ||
}, | ||
"scripts": { | ||
"clean": "rm -rf ./lib", | ||
"build": "npm run clean && npm run build:esm && npm run build:cjs", | ||
"build:esm": "tsc -p ./configs/tsconfig.esm.json && mv lib/esm/index.js lib/esm/index.mjs", |
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.
The renaming is unnecessary.
@@ -40,6 +31,7 @@ | |||
"keywords": [], | |||
"author": "", | |||
"devDependencies": { | |||
"@nomicfoundation/hardhat-toolbox": "^2.0.2", |
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.
Moved this to devDeps
as it wasn't browser friendly but isn't used in production code.
@@ -65,6 +56,8 @@ | |||
"@typechain/hardhat": "^6.1.2", | |||
"@types/chai": "^4.3.5", | |||
"@types/lodash": "^4.14.194", | |||
"ajv": "^8.12.0", |
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.
Replaced incompatible jsonschema
with https://github.com/ajv-validator/ajv
"jsonschema": "^1.4.1", | ||
"jsonwebtoken": "^9.0.0", | ||
"key-did-resolver": "1.4.0", | ||
"jose": "^5.2.4", |
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.
The jsonwebtoken library is targeting node enviroments. I replaced it with https://github.com/panva/jose
@@ -86,5 +78,8 @@ | |||
"overrides": { | |||
"flat": ">=5.0.1", | |||
"minimatch": ">=3.0.5" | |||
}, | |||
"browser": { | |||
"fs": false |
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.
This prevents bundler for containing incompatible fs
lib to browser enviroments.
@@ -1,5 +1,5 @@ | |||
import { DIDResolutionResult, DIDResolver, Resolver } from "did-resolver"; | |||
import {randomBytes} from "crypto" |
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.
crypto
is not browser-compatible.
@@ -29,6 +29,10 @@ export class HelperUtils { | |||
* Throws `ReadFileJsonFailureError` if reading or parsing fails | |||
*/ | |||
static async fileReaderJSON(location: string) { | |||
if (typeof window !== 'undefined') { | |||
// Check if we are in a browser env | |||
throw Error(`Operation 'fileReaderJSON' not supported in browser enviroment`) |
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.
Throwing error, if caller wants to use this function, in web enviroment.
@@ -97,7 +97,10 @@ export class JWTService implements SignatureService { | |||
* @return decoded JWT object {header, payload, signature} | |||
*/ | |||
decodeJWT(token: JWT) { |
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.
Here we could add a simpler decodePayload
function perhaps
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.
lgtm
Having also problems with ESM module, this PR is needed. @apratt3377 can you let us know what is blocking this PR from getting merged and shipped? |
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.
lgtm
Prior to this PR, the SDK wasn't working as expected when exported as an ESM module and several libraries were not compatible with browser enviroments.
Specifically the
exports
field inpackage.json
was confusing ESM consumers.There is a long thread about this nodejs/node#33460 and blog post that explains this issue more concisely here
Replaced non-compliant libraries with more browser-friendly ones and made appropriate changes.