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: json assertions #267

Merged
merged 19 commits into from
Jul 9, 2024
Merged

fix: json assertions #267

merged 19 commits into from
Jul 9, 2024

Conversation

DNR500
Copy link
Contributor

@DNR500 DNR500 commented Jul 8, 2024

Jira: LF-9087

We were have some issues in some of the dev tools that we support related to json assertions.

  • Next.js pages router would report error when trying to our ClientOnly method of importing the Widget
  • Remix would also report errors in its terminal output when trying to use the WIdgetSkeleton

We had originally tried to put json assertions in place but reverted the change as it caused problems with the use of Create React App.

In regards to this, this PR..

  • reintroduces the json assertions
  • adds a configuration work around to the create-react-app example
  • presents update nextjs (next v14) and nextjs-page-router (next v13) examples to document best usage in Pages and App router
  • Adds the WidgetSkeleton to the remix example

in addition..

  • Also adds a fix to the vite example for the connect button
  • Adds minor updates to the documentation in the examples
  • fixes some minor linting issue

Though this change also helps improve the usage of the WidgetSkeleton there are still some issues relating to its usage - these might be best explored in future tickets.

  • The WidgetSkeleton triggers some compilation errors with used in Next.js 13 and its App router (this doesn't happen in Next 14 or Next 13 Pages router - see notes the nextjs-page-router example)
  • Remix the WidgetSkeleton seems to only be usable in the fallback of the suspense block, usage in other place seems to cause issues in how the widget renders afterwards - this seems to only happen with remix

Testing
I've tested the Widget updates locally using the examples.

Release considerations

  • Once the released we will need to do an additional PR to bump version numbers in the examples folders

@DNR500 DNR500 added the WIP Work in progress label Jul 8, 2024
@DNR500 DNR500 changed the title Json assertions fix: json assertions Jul 9, 2024
@@ -1,7 +1,8 @@
import type { LiFiStepExtended } from '@lifi/sdk';
import { useEffect, useState } from 'react';
import { useTranslation } from 'react-i18next';
import { useTimer } from 'react-timer-hook';
import pkg from 'react-timer-hook';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added this as there was an error from one of the examples about this package being common js

@@ -25,8 +25,7 @@ export function WalletHeader() {
<Button
variant="contained"
disableElevation
// onClick={handleDisconnect}
onClick={() => connectAsync({ connector: connectors[0] })}
onClick={() => connectAsync({ connector: connectors[1] })}
Copy link
Contributor Author

@DNR500 DNR500 Jul 9, 2024

Choose a reason for hiding this comment

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

This wasn't work - this fix at least allow a user to connect using WalletConnect dialog

@DNR500 DNR500 marked this pull request as ready for review July 9, 2024 13:47
@chybisov chybisov merged commit 1c0c157 into main Jul 9, 2024
1 check passed
@chybisov chybisov deleted the json-assertions branch July 9, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants