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

[fix]: update configuration for exporting package #25

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

alexmylonas
Copy link

@alexmylonas alexmylonas commented Mar 6, 2024

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 in package.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.

"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",
Copy link
Author

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",
Copy link
Author

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",
Copy link
Author

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",
Copy link
Author

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
Copy link
Author

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"
Copy link
Author

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`)
Copy link
Author

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) {
Copy link
Author

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

Copy link
Contributor

@apratt3377 apratt3377 left a comment

Choose a reason for hiding this comment

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

lgtm

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.

None yet

2 participants