-
Notifications
You must be signed in to change notification settings - Fork 32
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
CommonJS-only setup #57
Conversation
# Conflicts: # README.md # apps/developers/package.json # apps/self.id/package.json # apps/self.id/src/auth.ts # apps/self.id/src/pages/_app.tsx # docs/modules/framework.md # docs/modules/multiauth.md # docs/modules/react.md # docs/modules/ui.md # package.json # packages/core/package.json # packages/framework/package.json # packages/framework/src/index.ts # packages/multiauth/package.json # packages/multiauth/src/components/Modal.tsx # packages/multiauth/src/components/ModalItem.tsx # packages/multiauth/src/connectors/fortmatic.ts # packages/multiauth/src/connectors/injected.ts # packages/multiauth/src/connectors/portis.ts # packages/multiauth/src/connectors/torus.ts # packages/multiauth/src/connectors/walletConnect.ts # packages/multiauth/src/index.ts # packages/multiauth/src/networks.ts # packages/multiauth/test/connectors.test.ts # packages/multiauth/test/hooks.test.tsx # packages/react/package.json # packages/react/src/index.ts # packages/react/src/storage.ts # packages/ui/package.json # packages/ui/src/index.ts # packages/ui/src/theme.ts # packages/ui/test/__snapshots__/AvatarPlaceholder.test.tsx.snap # packages/web/package.json # templates/next-notes-typescript/next-env.d.ts # templates/next-notes-typescript/next.config.js # templates/next-notes-typescript/package.json # templates/next-notes-typescript/src/model.json # templates/next-notes-typescript/src/pages/_app.tsx # templates/webpack-basic-typescript/package.json # templates/webpack-basic-typescript/webpack.config.js # templates/webpack-basic/package.json # templates/webpack-basic/webpack.config.js # yarn.lock
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/3box/self-id/8fbMH8CTrppy29PunyVoTe8tKFit |
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.
looks mostly good. Just a question around package versions.
@@ -5,7 +5,7 @@ | |||
"scripts": { | |||
"docusaurus": "docusaurus", | |||
"start": "docusaurus start", | |||
"build": "docusaurus build", | |||
"--build": "docusaurus 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.
Why this change?
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.
To disable it, there seems to be some issues with the Docusaurus build but as we're not currently using this, I don't think it's worth spending time fixing.
@@ -1,31 +1,33 @@ | |||
{ | |||
"name": "self.id-template-webpack-basic-typescript", | |||
"version": "0.1.0", | |||
"version": "0.0.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.
Why does this change from 0.1.0
to 0.0.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.
Just for consistency with the other templates, the version doesn't matter here, we just need to set one to avoid having npm warn about it
@@ -1,27 +1,29 @@ | |||
{ | |||
"name": "self.id-template-webpack-basic", | |||
"version": "0.1.0", | |||
"version": "0.0.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.
Same thing here?
Description
I think the issue with CommonJS imports in #53 is due to having dual CJS and ESM exports. As 3ID Connect only exports CJS, imports from ESM versions of the consuming packages fail.
The migration to ESM across the stack is ongoing but not ready yet, so in the meantime the alternative would be to go CJS-only to avoid this situation.
This mostly affects the image imports in multiauth, that require extra configuration now when using Next.js.
I also moved the
vite-basic
template and removed it from the list in the README, as it only supports ESM.How Has This Been Tested?